Properly handle alerts from the peer in SSL_read.

Properly handle alerts from the peer in SSL_read.

We currently return 0 on them because OpenSSL returns 0 with an entry in the
error queue. (See bug.) This is completely insane, but it's how the API works
for now.

Add a test which causes the peer to return a fatal error. Extend
SSLClientSocketTest.Read to assert on the final exit code. This gives test
coverage for when the peer cleanly shuts down the connection.

BUG=466303
Committed: https://crrev.com/e74aabdb96a388dd0f11c60552710fcfc6b86649
Cr-Commit-Position: refs/heads/master@{#321040}

[davidben, 2015-03-11 21:47:18]

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

[davidben, 2015-03-11 21:47:19]

IMPORTANT: When reviewing this, do not look at OpenSSL's documentation.
OpenSSL's documentation on this front is made entirely of lies. The only source
of truth is the implementation of SSL_get_error. That function's entire purpose
in life is to smooth over all of OpenSSL's insanity, while adding some of its
own. (See bug report.)

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

https://codereview.chromium.org/994373004/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1580: pending_read_ssl_error_,
err_tracer, &pending_read_error_info_);
This logic is totally incomprehensible and insane. next_result switches from
type ssl_read_return_value_t* to net_return_value_t*. (And, of course,
ssl_read_return_value_t* is a distinct type from ssl_get_error_return_value_t*.)

I'll rewrite it in a follow-up.

https://codereview.chromium.org/994373004/diff/1/net/ssl/openssl_ssl_util.h
File net/ssl/openssl_ssl_util.h (right):

https://codereview.chromium.org/994373004/diff/1/net/ssl/openssl_ssl_util.h#n...
net/ssl/openssl_ssl_util.h:55: // SSL_ERROR_ZERO_RETURN must be handled
externally.
This is important because I don't want this function to accidentally cause
something to report net::OK.

[Ryan Sleevi, 2015-03-17 01:01:28]

Submit patches to fix the documentation too?

LGTM

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

https://codereview.chromium.org/994373004/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1574: // SSL_ERROR_ZERO_RETURN.
Thanks, I conformed that ssl3_get_record does indeed call OPENSSL_PUT_ERROR, but
causes a zero return. This zero is propogated through ssl3_read_bytes, which
then goes through the TLS_protocol_method and up to the caller of SSL_read.

[davidben, 2015-03-17 01:18:51]

On 2015/03/17 01:01:28, Ryan Sleevi wrote:
> Submit patches to fix the documentation too?

I'll bring it up in my next upstream newsletter. I think really the semantics
need to be tightened up so 0 only happens on transport-level EOF.

[davidben, 2015-03-17 16:12:29]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-03-17 16:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/994373004/#ps20001 (title:
"rebase")

[commit-bot: I haz the power, 2015-03-17 16:12:45]

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

[commit-bot: I haz the power, 2015-03-17 21:46:45]

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

[commit-bot: I haz the power, 2015-03-17 21:46:46]

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

[davidben, 2015-03-17 23:50:08]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-03-17 23:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/994373004/#ps40001 (title:
"fix CrOS tests")

[commit-bot: I haz the power, 2015-03-17 23:51:12]

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

[commit-bot: I haz the power, 2015-03-18 01:04:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-18 01:05:09]

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