Tidy up SSL_read error handling.

Tidy up SSL_read error handling.

The old logic was difficult to follow with pointers and values switching from
SSL_read return values to net return values. This new formulation should
hopefully be clearer.

BUG=none

Committed: https://crrev.com/7e555daf95a1fbead12dc60fb0f7f1eaab9c22b7
Cr-Commit-Position: refs/heads/master@{#322179}

[davidben, 2015-03-11 22:18:04]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2015-03-11 22:18:04]

This sits on top of https://codereview.chromium.org/994373004/.

[Ryan Sleevi, 2015-03-17 00:33:14]

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:1550: } else if (ssl_ret <= 0) {
Why is this change correct?

Previously, this code only executed for cases where total_bytes_read !=
user_read_buf_len (per line 1544)

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:1575: if (total_bytes_read > 0 ||
user_read_buf_len_ == 0) {
How is it possible for user_read_buf_len to equal 0?

[davidben, 2015-03-20 20:46:04]

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:1550: } else if (ssl_ret <= 0) {
On 2015/03/17 00:33:14, Ryan Sleevi wrote:
> Why is this change correct?
> 
> Previously, this code only executed for cases where total_bytes_read !=
> user_read_buf_len (per line 1544)

Claim: Exactly one of ssl_ret <= 0 and total_bytes_read == user_read_buf_len_
are true.


In English:

ssl_ret <= 0 means the last SSL_read call failed. total_bytes_read ==
user_read_buf_len_ means we managed to read everything.

If the last SSL_read call failed, that means we hadn't read everything, tried to
read more, and didn't manage. Thus, total_bytes_read < user_read_buf_len_.

If we managed to read everything, that means the last SSL_read call must have
succeeded and we wouldn't have bothered trying to read more. This means ssl_ret
> 0.


In mindless symbol-manipulation:

Precondition: user_read_buf_len_ > 0. (See comment below.)

The for loop has exited so:
  ssl_ret <= 0 or total_bytes_read >= user_read_buf_len_.

SSL_read never returns anything larger than user_read_buf_len_ -
total_bytes_read, so total_bytes_read <= user_read_buf_len_. Thus we know:
  ssl_ret <= 0 or total_bytes_read == user_read_buf_len_.

So we know at least one is true. We need to show at most one is.

Assume both are:
  ssl_ret <= 0 and total_bytes_read == user_read_buf_len_

Thus, in the final iteration of the loop, ssl_ret <= 0 and line 1542 did not
run. So total_bytes_read and user_read_buf_len_ did not change.

That means that total_bytes_read == user_read_buf_len_ was true at the start of
the loop iteration. Thus the loop must have ran only once, otherwise we'd have
aborted the loop after the previous iteration,

If the loop ran only once and that iteration had ssl_ret <= 0, that means
total_bytes_read never changed, so user_read_buf_len_ = total_bytes_read = 0.
But we're assuming user_read_buf_len_ > 0. Contradiction.

----

Slightly less formally, this is just cleaning up the logic so that it's:

1. Convert the error code, if any, into a net error code with associated stuff.

2. If there's any data to return, return that first and defer the error.
Otherwise, return the error immediately.

https://codereview.chromium.org/1000813002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:1575: if (total_bytes_read > 0 ||
user_read_buf_len_ == 0) {
On 2015/03/17 00:33:14, Ryan Sleevi wrote:
> How is it possible for user_read_buf_len to equal 0?

I noticed that we don't actually have any checks that you don't try to Read()
with zero length. Though, tracing through BoringSSL, it looks like SSL_read
behaves weird too. So neither the old logic nor the new would really work.

I've just added a DCHECK for now. Seems SocketLibevent::Read also has one.

[Ryan Sleevi, 2015-03-24 23:22:49]

lgtm

[davidben, 2015-03-24 23:53:08]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-03-24 23:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000813002/40001

[commit-bot: I haz the power, 2015-03-25 04:34:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 04:34:28]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2015-03-25 15:22:59]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-03-25 15:23:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000813002/40001

[commit-bot: I haz the power, 2015-03-25 17:03:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-25 17:04:39]

Patchset 3 (id:??) landed as
https://crrev.com/7e555daf95a1fbead12dc60fb0f7f1eaab9c22b7
Cr-Commit-Position: refs/heads/master@{#322179}