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.

The new behavior is as follows:

- As before, on transport read error, BIO_read will fail after consuming the
  buffer. The read error is now preserved even when returned synchronously.

- On write error, future writes will fail as before. Unlike before, a write
  error does not prevent further data from the OS to be buffered into the read
  end of the BIO. Instead, on transport write failure, we continue to read
  from the transport and fill our read buffer. Whenever the buffer is empty,
  the write error is surfaced. This is so the consumer is still notified of
  a write failure on its final Write() call.

- When interpreting an OpenSSL error, return the saved transport read or
  write error when appropriate by determining if the OpenSSL operation
  failed reading from the BIO (SSL_ERROR_WANT_READ) or writing to it
  (writing to a closed BIO pair gives BIO_R_RESET_PIPE).

- On transport error, always pump the OpenSSL state machine first. This is
  so that, if OpenSSL provides its own error, that is used instead of the
  transport error.

- Always return the transport error on handshake failure. Move the fallback
  logic for ERR_CONNECTION_CLOSED and ERR_CONNECTION_RESET out of
  SSLClientSocketNSS and into HttpNetworkTransaction with the rest of the
  fallback logic.

- If, after there is no more to fallback (either we're at SSLv3 or got an
  inappropriate_fallback), ERR_CONNECTION_CLOSED or ERR_CONNECTION_RESET is
  sent, the user will now see an error page for one of those codes rather
  than ERR_SSL_PROTOCOL_ERROR.

Add a test to assert that transport errors are returned out of the handshake
and another to assert that TCP reset fallback behaves as expected.

BUG=372849, 341178

[davidben, 2014-06-06 23:31:51]

Sending this out so I'm no longer sitting on it.

This is kind of a giant monster CL. I can probably split out the fallback stuff.
It does have some user-visible behavior changes around error pages shown in
fallback cases since error codes are no longer mapped in SSLClientSocketNSS. I
think this is probably better? We preserve most transport errors on NSS, just
not the ones we happen to fallback on. It also matches Firefox's behavior. I can
avoid that by duplicating some fallback logic between the two backends if the
existing behavior is better. See last bullet point in description.

With all this stuff, I believe our behavior w.r.t. transport errors should be
unified between the two backends.

(Next up: experimenting with a NonBlockingSocket wrapper over our
(Stream)?Socket interface so that SSLClientSocketOpenSSL and SSLClientSocketNSS
can share that code and the transport halves of their respective state machines
can be driven by read/write calls from OpenSSL and NSS rather than externally
like we do right now.)

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1455: // transport error code out of
OpenSSL.
As an alternative, we can BIO_shutdown_wr(SSL_get_wbio(ssl_)) as before and then
rely on the fact that OpenSSL returns BIO_R_BROKEN_PIPE when you try to write
into a BIO you've already shutdown_wr'd.

This seems mildly preferable to me in so far as it's more consistent with how
reads are handled and arguably calling BIO_shutdown_wr(bio); BIO_write(bio); is
a misuse of the API, even though it does check for the case and return a
predictable error code.

Reads don't need any special hooking because the empty buffer behavior and error
behavior are compatible.

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:1585: // the write error bleeds into
the read.
This comment is referring to this block:

https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/ns...

While after this CL, both nss_memio and SSLClientSocketOpenSSL's BIO both have
the same "almost full-duplex except for the write error hack" behavior, we do
not use NSS internally in a full-duplex mode.

https://codereview.chromium.org/280853002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/280853002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:6582: #if !defined(OS_ANDROID)
I'm more than a little annoyed about this (and the below one) since Android is
our only automated OpenSSL test coverage right now. Though I've run it manually
on Linux OpenSSL builds. Also, the Connect_WithSynchronousError test will assert
that the transport error is preserved and the fallback logic no longer is
backend-specific.

I might amend this if I find a way to get a TCP reset out of the remote test
server next week.

[agl, 2014-06-09 17:23:34]

LGTM

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1455: // transport error code out of
OpenSSL.
On 2014/06/06 23:31:51, David Benjamin wrote:
> As an alternative, we can BIO_shutdown_wr(SSL_get_wbio(ssl_))

You mean when the write error is detected in TransportWriteComplete? I also
think that's slightly preferable because it avoids this callback, although I
wouldn't worry about it if you're on the fence.

[davidben, 2014-06-10 19:16:30]

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1455: // transport error code out of
OpenSSL.
On 2014/06/09 17:23:34, agl wrote:
> On 2014/06/06 23:31:51, David Benjamin wrote:
> > As an alternative, we can BIO_shutdown_wr(SSL_get_wbio(ssl_))
> 
> You mean when the write error is detected in TransportWriteComplete? I also
> think that's slightly preferable because it avoids this callback, although I
> wouldn't worry about it if you're on the fence.

Revised to keep the BIO_shutdown_wr and condition on BIO_R_RESET_PIPE.

[Ryan Sleevi, 2014-06-18 01:13:18]

Comments on CL description:

- On write error, future writes will fail as before. Unlike before, a write 
error does not prevent further data from the OS to be buffered into the read 
end of the BIO. Instead, the write failure/read feedback is applied whenever 
a BIO_read failed to return data or a read failure. This mirrors the 
nss_memio behavior. 


I find this unclear/confusing, as written.
1) I'm not sure what you mean by "read feedback"
2) This seems, as described, to create this weird timing thing where as long as
the peer is sending us data, and we're pulling it from the kernel fast enough,
we won't surface a write error as a read error. Then, suddenly, if no more data
is available, we'll explode.

I'd rather some description for consistency here. That is, do we drain all the
(previously internally buffered from the socket) data, and then fail? Do we
continue to grab data from transport_->Read() until... ERR_IO_PENDING? EOF?

I can see a lot of weirdness happening, no matter what, I just find it somewhat
confusing (and I'm trying to read this as someone who doesn't want to reverse
engineer nss_memio to figure out what you meant)


- In lieu of partially integrating net errors into the OpenSSL error system 
without the strings or function codes, extract them afterwards when 
interpreting the OpenSSL error. Use the SSL_ERROR_WANT_READ and 
SSL_ERROR_WANT_WRITE signals to determine whether the OpenSSL operation 
failed reading or writing from the BIO. 


I feel like this may need expansion / rewording. That is, again, from reading
the description, I'm not sure why/what you mean by "partially integrating net
errors into the OpenSSL error system" - or where/how/why that's relevant. Nor is
it clear why SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE signals are appropriate
for BIO failure, especially since other BIOs (most notably, BIO_file) are
perfectly capable of handling errno-styled results w/o issue.

That is, if OpenSSL can manage to be implemented atop a socket fd, why does or
does not our BIO behave like that? This description of errors hints at some
reason, but doesn't address.


- On transport error, always pump the OpenSSL state machine first. This is 
so that, if OpenSSL provides its own error, that is used instead of the 
transport error. 


I honestly don't know what this means, and will have to pay close attention to
the code. I don't know what you mean by "pump the OpenSSL state machine" - if
you mean calling SSL_read/SSL_write, or if you mean DoLoop(), or if you mean
something entirely different.


- Always return the transport error on handshake failure. Move the fallback 
logic for ERR_CONNECTION_CLOSED and ERR_CONNECTION_RESET out of 
SSLClientSocketNSS and into HttpNetworkTransaction with the rest of the 
fallback logic. 


1) SSLClientSocketNSS? [Just reading description, I would expect it to be
OpenSSL]
2) [Not having read the code yet] this doesn't strike me as correct.

[Ryan Sleevi, 2014-06-18 01:39:53]

Lots of splitting of mega patches will help.

In this case, splitting off the TestServer changes into a separate CL would be
good. I have not (yet) reviewed them.

A lot of this is comment-nits, which I apologize for. I'm trying to come at this
fresh, and without a ton of state mapped in, I'd like to hope it's clear and
understandable. And by that, I mean someone as slow as me can understand it ;)

I feel like there are several places where you hint at some alternate design or
approach X, or some other system Y. While it's good to be brief, it
unfortunately assumes your reader is familiar with the behaviours of X or Y,
which isn't always the case. Also, comments, like code, can be thought of as
having dependencies/coupling - referring to the behaviour of X in a comment
somewhat tightly couples you to X, so better to avoid that.

https://codereview.chromium.org/280853002/diff/220001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/280853002/diff/220001/net/http/http_network_t...
net/http/http_network_transaction.cc:1352: should_fallback = true;
For future reference, changes like this should really belong in a separate CL.

It's subtle, full of danger, and requires separate reasoning about to ensure
it's correctness. I'm aware that NSS and OpenSSL may have divergent behaviour
here, and you're fixing that behaviour, but that's not a strong enough
justification for lumping this in. It's perfectly OK to have CL chains.



Independent of that, I do wonder whether this logic belongs associated with 1324
- that is, in both cases, we want to do minimal fallback.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:345: transport_write_error_(0),
Shouldn't this be ERR_IO_PENDING ?

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:790: }
No need for these braces.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1023: bool network_moved;
I'm having trouble reasoning why this deletion is OK. I'm aware you said you
intentionally "pump the state", but it's unclear why this is desirable or
better.

That is, DoPayloadRead() will now fast-fail, but you're still going to force a
DoTransportIO call, and I don't know if that's actually a good thing, especially
if there's nothing to do.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1249: // Route transport failures around
OpenSSL. Use |ssl_error| to determine
It's unclear what you mean by "around OpenSSL"

That is, one common, colloquial usage is "through" - eg: navigating it through
the various internal subsystems of OpenSSL.

Another is that you mean to "avoid" OpenSSL entirely.

It's further confusing through your last sentence, which suggests it's done "in
lieu of through", and what the reader can expect that might mean.

Put differently: Don't assume your reader is familiar with your alternative
designs (which I think you're hinting at using BIO to put an error on the
stack?) if you're going to mention them. At least provide a little more detail
:)

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1258: // never do. This matches the fix
for https://crbug.com/249848 in NSS.
I think I'm getting confused by your actor model, which itself is confused
because of BIO.

"If OpenSSL wanted read data" really means "If the caller attempted to read
data", correct? If so, that latter phrasing makes it clearer, since the OpenSSL
model, three different things are reading - the caller from OpenSSL, OpenSSL
from the BIO, the BIO from the socket - and it's not clear at what phase you're
reporting this error.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1265: // queue. Use this to signal
transport write failure.
"On transport write failure, ... Use this to signal transport write failure" -
doesn't quite parse here either. I'm not sure what the rewording is, but more is
better.

"If the underlying transport socket was unable to write data, SSL_read/SSL_write
will add BIO_R_BROKEN_PIPE to the error queue. Use this to signal to the caller
that this socket is no longer writable"

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1269: // reported in a future write or
read.
s/write or read/call to XXX or YYY/

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:189: // still readable.
Well, you document this, but then you use "0" throughout the .cc file, not
ERR_IO_PENDING.

[davidben, 2014-06-18 17:25:05]

On 2014/06/18 01:39:53, Ryan Sleevi wrote:
> Lots of splitting of mega patches will help.
> 
> In this case, splitting off the TestServer changes into a separate CL would be
> good. I have not (yet) reviewed them.

Sure. I'll address the comments so far on this CL and work on splitting this up.

[davidben, 2014-06-18 18:09:42]

On 2014/06/18 01:13:18, Ryan Sleevi wrote:
> Comments on CL description:
> 
> - On write error, future writes will fail as before. Unlike before, a write 
> error does not prevent further data from the OS to be buffered into the read 
> end of the BIO. Instead, the write failure/read feedback is applied whenever 
> a BIO_read failed to return data or a read failure. This mirrors the 
> nss_memio behavior. 
> 
> 
> I find this unclear/confusing, as written.
> 1) I'm not sure what you mean by "read feedback"
> 2) This seems, as described, to create this weird timing thing where as long
as
> the peer is sending us data, and we're pulling it from the kernel fast enough,
> we won't surface a write error as a read error. Then, suddenly, if no more
data
> is available, we'll explode.
> 
> I'd rather some description for consistency here. That is, do we drain all the
> (previously internally buffered from the socket) data, and then fail? Do we
> continue to grab data from transport_->Read() until... ERR_IO_PENDING? EOF?
> 
> I can see a lot of weirdness happening, no matter what, I just find it
somewhat
> confusing (and I'm trying to read this as someone who doesn't want to reverse
> engineer nss_memio to figure out what you meant)

Yeah, (2) is nss_memio's behavior. It's really really weird. I really really
dislike that artifact of nss_memio, but I don't see a terribly sane way to
reconcile wanting full-duplex and plumbing write errors back up to reads. This
is a consequence of wanting to preserve full-duplex behavior where write
failures do not interrupt the read pipeline. (I believe you said the motivation
was to not lose any last chunks of read data buffered by the OS and whatnot.)

Rephrased to hopefully be a little clearer.


> - In lieu of partially integrating net errors into the OpenSSL error system 
> without the strings or function codes, extract them afterwards when 
> interpreting the OpenSSL error. Use the SSL_ERROR_WANT_READ and 
> SSL_ERROR_WANT_WRITE signals to determine whether the OpenSSL operation 
> failed reading or writing from the BIO. 
> 
> 
> I feel like this may need expansion / rewording. That is, again, from reading
> the description, I'm not sure why/what you mean by "partially integrating net
> errors into the OpenSSL error system" - or where/how/why that's relevant. Nor
is
> it clear why SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE signals are appropriate
> for BIO failure, especially since other BIOs (most notably, BIO_file) are
> perfectly capable of handling errno-styled results w/o issue.
> 
> That is, if OpenSSL can manage to be implemented atop a socket fd, why does or
> does not our BIO behave like that? This description of errors hints at some
> reason, but doesn't address.

As I understand, the way you're supposed to handle errors when atop a socket fd
is that SSL_get_error will return SSL_ERROR_SYSCALL which means "got some error
outside openssl, not my problem". It's returned if either the top error is in
the ERR_LIB_SYS library or if there is no error on the error stack and OpenSSL
has no more useful information to give like SSL_ERROR_WANT_READ. In that case,
you're supposed to look at errno.

To be able to mimic either of those exactly how another BIO would do it, we have
to either route our errors through OpenSSL's error system and put them on the
stack (actually earlier patch sets did just that), but Adam said he'd prefer not
to do that since we'd only be partially integrating our error codes into
OpenSSL. The other option is to act like errno. That means saving the error of
the last failed BIO operation. Note that this is different from saving the
transport_read_error_ and transport_write_error_. Those are for saving the
current state of the socket and don't change whether the last failed BIO
operation was a read or a write.

Either way, we need to hook into BIO_read or BIO_write calls. This means either
a custom BIO implementation or a callback. A previous version used a callback,
but Adam thought not using the callback was preferable.

I've rewritten that section to hopefully be clearer.

(Aside: I actually would like to experiment with implementing this as a custom
BIO sometime anyway and make the BIO drive StreamSocket when BIO_read or
BIO_write is called. We'll see how it works. My hope is that it'll avoid having
the SSLClientSocket state machine know about how DoPayloadRead and BufferSend
are supposed to interact. Would also make something like
https://codereview.chromium.org/255143002/ simpler. But I'd rather get the
behavior aligned first before playing with that refactor.)

> - On transport error, always pump the OpenSSL state machine first. This is 
> so that, if OpenSSL provides its own error, that is used instead of the 
> transport error. 
> 
> 
> I honestly don't know what this means, and will have to pay close attention to
> the code. I don't know what you mean by "pump the OpenSSL state machine" - if
> you mean calling SSL_read/SSL_write, or if you mean DoLoop(), or if you mean
> something entirely different.

This is removing the if (result < 0) check at the top of DoReadLoop. I'll split
this out into a separate CL and/or drop it. SSLClientSocketNSS does this too, so
it's not actually relevant towards aligning them. I think it's still correct,
but it should be done separately and in both state machines. Probably with a
test too... this is so that if the SSL implementation ever, when receiving a
transport error, decides to return a different error, we return the one the SSL
implementation expected. We seem to be a little inconsistent here depending on
whether SSLClientSocket::Read() returns synchronously or not.

The one place I know of where this is relevant is that OpenSSL ignores transport
errors after a close_notify is received, so it's not actually that important.
Not sure why I lumped it into this CL, to be honest.

I've not bothered to revise that description since it will be split out.

> - Always return the transport error on handshake failure. Move the fallback 
> logic for ERR_CONNECTION_CLOSED and ERR_CONNECTION_RESET out of 
> SSLClientSocketNSS and into HttpNetworkTransaction with the rest of the 
> fallback logic. 
> 
> 
> 1) SSLClientSocketNSS? [Just reading description, I would expect it to be
> OpenSSL]
> 2) [Not having read the code yet] this doesn't strike me as correct.

NSS is correct. I guess you've read the code now, but while we fallback on
ERR_CONNECTION_CLOSED and ERR_CONNECTION_RESET, we do so not at
HttpNetworkTransaction level, where half of our error codes are checked, but in
SSLClientSocketNSS by mapping them to ERR_SSL_PROTOCOL_ERROR (which
HttpNetworkTransaction will fallback on) when we want to fallback. This is
merging the two together so that SSLClientSocketOpenSSL can share that logic and
gain the "fallback on reset, but only down to TLS 1.0" behavior.

When I split this up, this is one of the pieces I'll pull out separately.

[davidben, 2014-06-18 22:27:23]

(Oops, forgot to hit publish.)

https://codereview.chromium.org/280853002/diff/220001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/280853002/diff/220001/net/http/http_network_t...
net/http/http_network_transaction.cc:1352: should_fallback = true;
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> For future reference, changes like this should really belong in a separate CL.
> 
> It's subtle, full of danger, and requires separate reasoning about to ensure
> it's correctness. I'm aware that NSS and OpenSSL may have divergent behaviour
> here, and you're fixing that behaviour, but that's not a strong enough
> justification for lumping this in. It's perfectly OK to have CL chains.

Yeah, I'll split this one out.

> Independent of that, I do wonder whether this logic belongs associated with
1324
> - that is, in both cases, we want to do minimal fallback.

1324?

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:345: transport_write_error_(0),
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> Shouldn't this be ERR_IO_PENDING ?

Fixed docs.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:790: }
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> No need for these braces.

Done. I think that was a remnant of when it was ERR_IO_PENDING and I had to wrap
the conditional.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1023: bool network_moved;
On 2014/06/18 01:39:53, Ryan Sleevi wrote:
> I'm having trouble reasoning why this deletion is OK. I'm aware you said you
> intentionally "pump the state", but it's unclear why this is desirable or
> better.
> 
> That is, DoPayloadRead() will now fast-fail, but you're still going to force a
> DoTransportIO call, and I don't know if that's actually a good thing,
especially
> if there's nothing to do.

Hrm. Good point that there's an extra DoTransportIO call. This is the thing I
mentioned I'll split out and/or drop in comment #7 so I'll leave it as is here
for context and we'll see what happens to the split-out CL.

(this bit:
> - On transport error, always pump the OpenSSL state machine first. This is 
> so that, if OpenSSL provides its own error, that is used instead of the 
> transport error. 
)

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1249: // Route transport failures around
OpenSSL. Use |ssl_error| to determine
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> It's unclear what you mean by "around OpenSSL"
> 
> That is, one common, colloquial usage is "through" - eg: navigating it through
> the various internal subsystems of OpenSSL.
> 
> Another is that you mean to "avoid" OpenSSL entirely.
> 
> It's further confusing through your last sentence, which suggests it's done
"in
> lieu of through", and what the reader can expect that might mean.
> 
> Put differently: Don't assume your reader is familiar with your alternative
> designs (which I think you're hinting at using BIO to put an error on the
> stack?) if you're going to mention them. At least provide a little more detail
> :)

Rewrote comment.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1258: // never do. This matches the fix
for https://crbug.com/249848 in NSS.
On 2014/06/18 01:39:53, Ryan Sleevi wrote:
> I think I'm getting confused by your actor model, which itself is confused
> because of BIO.
> 
> "If OpenSSL wanted read data" really means "If the caller attempted to read
> data", correct? If so, that latter phrasing makes it clearer, since the
OpenSSL
> model, three different things are reading - the caller from OpenSSL, OpenSSL
> from the BIO, the BIO from the socket - and it's not clear at what phase
you're
> reporting this error.

I actually do mean if OpenSSL wanted to read data.

My model here is that the caller wants to read/write data from
SSLClientSocketOpenSSL, so we read/write from OpenSSL. In response to that,
OpenSSL may decide to read/write from OpenSSL's end of the BIO. Because of
renegotiations, OpenSSL's operations may not necessarily match ours. Reading
from OpenSSL's end from the BIO conceptually should then read from the
transport.

And then all these state machine complexities are to deal with us translating
back and forth between asynchronous + completion callback vs. non-blocking +
retry when ready.

It's kind of a mess because that last link---reading from BIO gets results from
reads on the transport---is managed by SSLClientSocketOpenSSL as BIO itself is a
BIO pair and gets data stuffed into it out-of-band. And also because there's no
good way to stuff error codes from the transport into the BIO.

This is all simulating what would have happened if we had errnos and used
SSL_ERROR_SYSCALL but does so without registering a BIO callback.

(Aaargh, all this terminology. It doesn't help that "write to the BIO" is
ambiguous because the crazy thing has two ends.)

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1265: // queue. Use this to signal
transport write failure.
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> "On transport write failure, ... Use this to signal transport write failure" -
> doesn't quite parse here either. I'm not sure what the rewording is, but more
is
> better.
> 
> "If the underlying transport socket was unable to write data,
SSL_read/SSL_write
> will add BIO_R_BROKEN_PIPE to the error queue. Use this to signal to the
caller
> that this socket is no longer writable"

Rephrased it a little.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1269: // reported in a future write or
read.
On 2014/06/18 01:39:52, Ryan Sleevi wrote:
> s/write or read/call to XXX or YYY/

Elaborated.

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:189: // still readable.
On 2014/06/18 01:39:53, Ryan Sleevi wrote:
> Well, you document this, but then you use "0" throughout the .cc file, not
> ERR_IO_PENDING.

Oops. Done.

[Ryan Sleevi, 2014-06-19 00:27:06]

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1258: // never do. This matches the fix
for https://crbug.com/249848 in NSS.
On 2014/06/18 22:27:22, David Benjamin wrote:
> I actually do mean if OpenSSL wanted to read data.

Ok, hereby declaring a moratorium on the personification of OpenSSL :)

For clarity sake, let's refer to the actual reader of the BIO - the SSL* - so
that it's clear. If OpenSSL has some internal BIO doing things, then we call it
a BIO. etc.

Sound less confusing? :)

as for "write to the BIO", the term can be clarified by
"write outgoing ciphertext"
"write incoming socket data"

https://codereview.chromium.org/280853002/diff/240001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/280853002/diff/240001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1250: // |errno|, or the equivalent, for
the underlying transport error. This would
Having trouble parsing one bit of this

// Normal OpenSSL error-handling works by having
// |ssl_error| being set to SSL_ERROR_SYSCALL. The application
// is then expected to first check the error queue, and, if
// empty, checking |errno| or equivalent for the underlying
// transport error.
//
// To do this with the BIO would require hooking into the
// BIO_read/BIO_write calls to OpenSSL's half of |transport_bio_|, in order to
determine whether to
// extract |transport_read_error_| or |transport_write_error_|

(side note: Not sure I fully grok the "require hooking into BIO_read/BIO_write
calls to OpenSSL". Are you saying OpenSSL would have to be modified? You'd have
to create a custom BIO? Something else?)

https://codereview.chromium.org/280853002/diff/240001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1251: // require hooking into BIO_read
and BIO_write calls to OpenSSL's end of
Ok, let's use "SSL*" rather than "OpenSSL" for portions where you're talking
about OpenSSL having the SSL* call BIO_read/BIO_write.

I think that means line 1251, 1260, and 1272/1273/1277/1280?

[davidben, 2014-07-02 21:06:46]

This CL has gotten split up into several and the error handling made saner in
https://codereview.chromium.org/367963007/. Closing this instance.