Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10543069:
Fix SSLSocketAdapter to handle asynchronous writes appropriately. (Closed)
|
Created:
8 years, 6 months ago by Sergey Ulanov Modified:
8 years, 6 months ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix SSLSocketAdapter to handle asynchronous writes appropriately.
Also fixed some other bugs, particularly asynchronous reads were not
handled properly in some cases.
BUG=129658
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141946
Patch Set 1 #
Total comments: 17
Patch Set 2 : #Patch Set 3 : #
Total comments: 20
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Messages
Total messages: 11 (0 generated)
ping
http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:88: SetError(EINVAL); You're avoiding just returning whatever error caused SSLSTATE_ERROR because it might not be something that makes sense for a Send() call to return? http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:90: } else if (ssl_state_ != SSLSTATE_CONNECTED) { nit: Using "else" here makes this harder to read. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:91: return AsyncSocketAdapter::Send(buf, len); Add a comment to explain why we propagate the Send() to the underlying implementation if we're not connected yet. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:134: LOG(ERROR) << "Error when reading from SSL socket " << result; nit: not need for "when" here http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:146: static_cast<int>(len)); You'll need {} in this case, since you're declaring case-scoped variables. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:146: static_cast<int>(len)); nit: If len > read_buffer_size_ - read_buffer_position_ then we should ideally schedule a Read() to get more data, rather than wait til we hit the end of the current buffer, to maximize throughput. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:148: read_buffer_position_ += size; Why use a buffer position rather than creating a DrainableIOBuffer when we first hit the IOSTATE_COMPLETE state? http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:183: ssl_state_ = SSLSTATE_ERROR; nit: It's confusing that you have read_state_, write_state_ and ssl_state_, but that ssl_state_ is the only one that contains the ERROR flag for all three... http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:206: DCHECK_GT(write_buffer_->BytesRemaining(), 0); nit: DCHECK |write_state_| is not IO_PENDING?
http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:88: SetError(EINVAL); On 2012/06/11 21:21:35, Wez wrote: > You're avoiding just returning whatever error caused SSLSTATE_ERROR because it > might not be something that makes sense for a Send() call to return? Correct. Send() returns one of the error codes defined in net/base/net_errors.h. We would have to convert it to POSIX error code. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:90: } else if (ssl_state_ != SSLSTATE_CONNECTED) { On 2012/06/11 21:21:35, Wez wrote: > nit: Using "else" here makes this harder to read. Done. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:91: return AsyncSocketAdapter::Send(buf, len); On 2012/06/11 21:21:35, Wez wrote: > Add a comment to explain why we propagate the Send() to the underlying > implementation if we're not connected yet. Done. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:134: LOG(ERROR) << "Error when reading from SSL socket " << result; On 2012/06/11 21:21:35, Wez wrote: > nit: not need for "when" here Done. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:146: static_cast<int>(len)); On 2012/06/11 21:21:35, Wez wrote: > You'll need {} in this case, since you're declaring case-scoped variables. Done. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:148: read_buffer_position_ += size; On 2012/06/11 21:21:35, Wez wrote: > Why use a buffer position rather than creating a DrainableIOBuffer when we first > hit the IOSTATE_COMPLETE state? Drainable buffer doesn't allow truncating it. Replaced it with GrowableIOBuffer. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:183: ssl_state_ = SSLSTATE_ERROR; On 2012/06/11 21:21:35, Wez wrote: > nit: It's confusing that you have read_state_, write_state_ and ssl_state_, but > that ssl_state_ is the only one that contains the ERROR flag for all three... Repalaced write_state_ and read_state_ with write_pending_ and read_pending_ flags. http://codereview.chromium.org/10543069/diff/1/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:206: DCHECK_GT(write_buffer_->BytesRemaining(), 0); On 2012/06/11 21:21:35, Wez wrote: > nit: DCHECK |write_state_| is not IO_PENDING? Done.
ping
http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:91: // Propagate the call to underlying socket if SSL is not connected yet. This explains what you're doing, but not _why_? Why is it appropriate to propagate the call to the underlying socket? Surely that means sending unencrypted data? When would we want to do that? Shouldn't we buffer the data until SSL is connected, or return EWOULDBLOCK? http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:122: return -1; nit: Define a constant for the error return-valie that libjingle expects? http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:127: if (read_buffer_) { nit:Add a comment to clarify that this is the case where we have some previously-read data buffered that we can return immediately to the caller? http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:132: if (!read_buffer_->RemainingCapacity()) nit: Inconsistent use of {} in ifs. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:135: if (size == static_cast<int>(len)) nit: Ditto http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:140: // have left. nit: Suggest "If we didn't fill the caller's buffer then dispatch a new Read() now, in case there's already more data ready to return to them." http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:151: base::Bind(&SSLSocketAdapter::OnRead, base::Unretained(this))); Add a comment before this block to clarify that this is the case where we had too little data, or no data at all, with which to satisfy the entire read. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:152: if (result >= 0) nit: Inconsistent use of {} on if http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:159: } else { I don't think this needs to be in an else, since the IO_PENDING case returns? http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:223: while (write_buffer_ && !write_pending_) { I'd suggest structuring this as a while (true) and returning in the IO_PENDING, !BytesRemaining() and (I think?) result < 0 cases. You could also put the DidConsume case first, with a continue, so that once you're past that point you know you're going to return.
http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:91: // Propagate the call to underlying socket if SSL is not connected yet. On 2012/06/13 00:48:57, Wez wrote: > This explains what you're doing, but not _why_? Why is it appropriate to > propagate the call to the underlying socket? Surely that means sending > unencrypted data? When would we want to do that? Shouldn't we buffer the data > until SSL is connected, or return EWOULDBLOCK? XMPP connections are not encrypted until <starttls> command is received, and the XMPP stack needs to be able to send/receive unencrypted data until then. Once connection is upgraded to TLS the upper layer calls StartSSL() that creates SSL layer. Updated the comment. Also changed condition to ssl_state_ == SSLSTATE_NONE, as sending any data here when we are WAIT state would be incorrect. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:122: return -1; On 2012/06/13 00:48:57, Wez wrote: > nit: Define a constant for the error return-valie that libjingle expects? Don't think it is necessary. The current code is consistent with libjingle code and with some parts of net/ http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:127: if (read_buffer_) { On 2012/06/13 00:48:57, Wez wrote: > nit:Add a comment to clarify that this is the case where we have some > previously-read data buffered that we can return immediately to the caller? Done. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:132: if (!read_buffer_->RemainingCapacity()) On 2012/06/13 00:48:57, Wez wrote: > nit: Inconsistent use of {} in ifs. It is consistent - all other single-line ifs in this file don't have {} http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:135: if (size == static_cast<int>(len)) On 2012/06/13 00:48:57, Wez wrote: > nit: Ditto Ditto http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:140: // have left. On 2012/06/13 00:48:57, Wez wrote: > nit: Suggest "If we didn't fill the caller's buffer then dispatch a new Read() > now, in case there's already more data ready to return to them." Done. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:151: base::Bind(&SSLSocketAdapter::OnRead, base::Unretained(this))); On 2012/06/13 00:48:57, Wez wrote: > Add a comment before this block to clarify that this is the case where we had > too little data, or no data at all, with which to satisfy the entire read. Done. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:152: if (result >= 0) On 2012/06/13 00:48:57, Wez wrote: > nit: Inconsistent use of {} on if It is consistent - all other single-line ifs in this file don't have {} http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:159: } else { On 2012/06/13 00:48:57, Wez wrote: > I don't think this needs to be in an else, since the IO_PENDING case returns? Done. http://codereview.chromium.org/10543069/diff/1003/remoting/jingle_glue/ssl_so... remoting/jingle_glue/ssl_socket_adapter.cc:223: while (write_buffer_ && !write_pending_) { On 2012/06/13 00:48:57, Wez wrote: > I'd suggest structuring this as a while (true) and returning in the IO_PENDING, > !BytesRemaining() and (I think?) result < 0 cases. You could also put the > DidConsume case first, with a continue, so that once you're past that point you > know you're going to return. Done.
lgtm http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:140: // If we didn't fill the callers buffer then dispatch a new nit: callers -> caller's http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:159: SetError(EWOULDBLOCK); nit: Strictly you shouldn't set EWOULDBLOCK unless you're going to return -1 - consider rephrasing this as if (bytes_read) {} else {}
http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... File remoting/jingle_glue/ssl_socket_adapter.cc (right): http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:140: // If we didn't fill the callers buffer then dispatch a new On 2012/06/13 18:32:01, Wez wrote: > nit: callers -> caller's Done. http://codereview.chromium.org/10543069/diff/5/remoting/jingle_glue/ssl_socke... remoting/jingle_glue/ssl_socket_adapter.cc:159: SetError(EWOULDBLOCK); On 2012/06/13 18:32:01, Wez wrote: > nit: Strictly you shouldn't set EWOULDBLOCK unless you're going to return -1 - > consider rephrasing this as if (bytes_read) {} else {} Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10543069/6004
Change committed as 141946 |
