Preserve transport errors for OpenSSL sockets.

Preserve transport errors for OpenSSL sockets.

This makes the OpenSSL BIO pair behave like nss_memio with respect to errors,
eliminating many discrepancies between the two backends in
ssl_client_socket_unittest.cc. (While adding one as it exposes a difference in
how OpenSSL and NSS behave internally.) This also makes our fallback behavior
on TCP reset match; in NSS we take care to only fall back to TLS 1, but our
OpenSSL code falls back all the way to SSL3.

We save transport errors and install a BIO callback to return those errors
through the OpenSSL error queue when OpenSSL's SSL implementation attempts to
read or write to the transport BIO.

BUG=372849, 341178
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283013

[davidben, 2014-07-02 18:30:11]

And the last piece of https://codereview.chromium.org/280853002/. Fallback stuff
has been split out, and, now that we have OpenSSLPutNetError, this is much
saner. (Though it does involve a BIO callback.)

[Ryan Sleevi, 2014-07-07 22:36:35]

wtc: Something odd, noted by David below, makes me wonder if we should change
NSS to match. Then again, it may be even more untested NSS code, so perhaps the
answer is no.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:664: BIO_set_callback_arg(ssl_bio,
reinterpret_cast<char*>(this));
Dang. So for now this is fine, but this would seem it would preclude the BIO
ever outliving the socket, right? I guess more of a note for havaard, though
that CL is stalled on BoringSSL figuring out the BIO semantics.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1286: result = ERR_CONNECTION_CLOSED;
aside: It does seem unfortunate (read: potentially dangerous) that we treat an
incomplete transport failure as a graceful TLS failure.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1290: // read on transport_bio_'s peer.
This comment is no longer correct. <0 != 0

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1444: if (cmd ==
(BIO_CB_READ|BIO_CB_RETURN) && retvalue <= 0) {
Any sanity DCHECKs on argp/argi/argl needed?

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1444: if (cmd ==
(BIO_CB_READ|BIO_CB_RETURN) && retvalue <= 0) {
if cmd == BIO_cb_return(BIO_CB_READ) ?

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1479: return socket->BIOCallback(bio,
cmd, argp, argi, argl, retvalue);
CHECK(socket) before hand?

Save us running off into the land of weirdness.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:160: long retvalue);
1) Document
2) Naming - In nss.cc, we use static+"Callback" (except where the one place we
use "Handler" :/). Perhaps BioCallback + HandleBioCallback (or something more
meaningful than "HandleBioCallback", to *actually* describe what it does)?

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:1764: // the write error stops future
reads.
wtc: HERE

[davidben, 2014-07-08 00:03:35]

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:664: BIO_set_callback_arg(ssl_bio,
reinterpret_cast<char*>(this));
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> Dang. So for now this is fine, but this would seem it would preclude the BIO
> ever outliving the socket, right? I guess more of a note for havaard, though
> that CL is stalled on BoringSSL figuring out the BIO semantics.

Hrm, that's true. If we ever need to cross that bridge, we'd want to move
transport_read_error_ and transport_write_error_ to the BIO's callback arg. Or
maybe just auxiliary data on the BIO with a custom BIO_METHOD.

This makes some sense since the equivalent fields are stored internally in the
nss_memio.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1286: result = ERR_CONNECTION_CLOSED;
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> aside: It does seem unfortunate (read: potentially dangerous) that we treat an
> incomplete transport failure as a graceful TLS failure.

Yeah, I was very surprised when that came up in the original iteration of this
code. We should have a DCHECK now though; OpenSSLPutNetError will NOTREACHED()
if you ever pass 0 to it.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1290: // read on transport_bio_'s peer.
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> This comment is no longer correct. <0 != 0

Done.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1444: if (cmd ==
(BIO_CB_READ|BIO_CB_RETURN) && retvalue <= 0) {
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> Any sanity DCHECKs on argp/argi/argl needed?

Not especially I don't think? For BIO_CB_READ and BIO_CB_WRITE, argp ends up
being the buffer, argi the size of the buffer, and argl 0.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1444: if (cmd ==
(BIO_CB_READ|BIO_CB_RETURN) && retvalue <= 0) {
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> if cmd == BIO_cb_return(BIO_CB_READ) ?

Hrm. That's been nuked in Boring. (Plus it isn't even used internally in
OpenSSL.)

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1479: return socket->BIOCallback(bio,
cmd, argp, argi, argl, retvalue);
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> CHECK(socket) before hand?
> 
> Save us running off into the land of weirdness.

Done.

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:160: long retvalue);
On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> 1) Document
> 2) Naming - In nss.cc, we use static+"Callback" (except where the one place we
> use "Handler" :/). Perhaps BioCallback + HandleBioCallback (or something more
> meaningful than "HandleBioCallback", to *actually* describe what it does)?

OpenSSL thus far uses the same name for both, but everything else is hanging off
SSLContext. Renamed to hopefully be clearer.

[wtc, 2014-07-09 15:55:27]

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:1764: // the write error stops future
reads.

On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> wtc: HERE

I think we should fix this. This shouldn't be too hard.

David, have you looked into this? I think we can have ssl_SecureRecv go on to
DoRecv even if the ssl_SendSavedWriteData call fails. But we may need to save
the error code of the ssl_SendSavedWriteData call in sslSocket and report that
error at the "right time", rather than calling ssl_SendSavedWriteData again.

[davidben, 2014-07-09 16:00:08]

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/367963007/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:1764: // the write error stops future
reads.
On 2014/07/09 15:55:27, wtc wrote:
> 
> On 2014/07/07 22:36:34, Ryan Sleevi wrote:
> > wtc: HERE
> 
> I think we should fix this. This shouldn't be too hard.
> 
> David, have you looked into this? I think we can have ssl_SecureRecv go on to
> DoRecv even if the ssl_SendSavedWriteData call fails. But we may need to save
> the error code of the ssl_SendSavedWriteData call in sslSocket and report that
> error at the "right time", rather than calling ssl_SendSavedWriteData again.

I haven't looked at it much, apart from finding the bit in the code that does
this. What if we set SSL_ENABLE_FDX? I dunno what else it does or if that logic
is actually desirable to begin with.

[Ryan Sleevi, 2014-07-14 18:10:32]

LGTM. I don't think we'll be using _FDX, as a (mostly) untested codepath with
it's own set of issues.

Odd that the OpenSSL doesn't try a similar flush.

[davidben, 2014-07-14 18:26:33]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-14 18:27:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/367963007/40001

[commit-bot: I haz the power, 2014-07-14 20:43:45]

Change committed as 283013