Close SSLClientSocketOpenSSL cleanly if the transport was closed.

Close SSLClientSocketOpenSSL cleanly if the transport was closed.

Servers do not reliably send close_notify. This regressed in
https://codereview.chromium.org/367963007 which had a side effect of making us
sensitive to this.

BUG=422246
Committed: https://crrev.com/be6ce7ecd6d31721576d39e65d74fdbda65d48f1
Cr-Commit-Position: refs/heads/master@{#300308}

[davidben, 2014-10-14 20:31:05]

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

[davidben, 2014-10-14 20:31:06]

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

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1349: int *next_result = &rv;
This logic (and similar logic in SSLClientSocketNSS) is incredibly convoluted
and next_result switches from type openssl_return_t* to net_return_t* part-way
through.  I'm going to leave cleaning that up to a follow-up so this can be
merged. It's orthogonal to this change.

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1500: result = ERR_CONNECTION_CLOSED;
It is a little annoying that this fix is undoing this canonicalization. The
awkward thing is that we /do/ want ERR_CONNECTION_CLOSED if it shuts off during
the handshake, but we /don't/ want it if it shuts off during a read.

Another option would be to:

- Remove this mapping. Instead, transport_read_error_ now stores 0 for EOF and
we pick some other value (ERR_IO_PENDING?) to signal that it's still readable.
Probably want to do the same thing to transport_write_error_. Then arrange for
BIO_read of the SSL* end of transport_bio_ to return 0 if the underlying
transport was closed, and -1 + OpenSSLPutNetError on error. This could either by
done by calling SSL_shutdown_wr or adjusting the callback. Either way,
transport_read_error_ needs to be adjusted for the BufferRecv() guard in line
879.

- This would make the SSL_read code DTRT because we'd get a zero return.
SSL_get_error would return SSL_ERROR_ZERO_RETURN or SSL_ERROR_SYSCALL if we
cared to distinguish close_notify but we don't.

- However, getting SSL_do_handshake right becomes tricky. (Before the original
CL, we didn't bother because everthing turned into ERR_SSL_PROTOCOL_ERROR.)
OpenSSL just bubbles the BIO return code up if it's 0 or -1. BUT it also returns
0 if you get a close_notify during the handshake, which should be
ERR_SSL_PROTOCOL_ERROR. The distinguishing factor is one gives
SSL_ERROR_ZERO_RETURN in SSL_get_error and the other SSL_ERROR_SYSCALL. So I
believe we need to first SSL_get_error, special-case rv == 0 && err ==
SSL_ERROR_SYSCALL to ERR_CONNECTION_CLOSED, and then do the normal thing.

This is all kind of a mess and it seems plumbing EOF through the error codepath
and mapping back in Read is simpler, but I can do the other one if you prefer.

[Ryan Sleevi, 2014-10-17 20:53:00]

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

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1363: // ERR_CONNECTION_CLOSED cannot be
treated as fatal.
This comment needs explaining (or my flu meds need to not be as strong). Namely,
you're relying on a lot of internal context in your head for this comment that
is lost in the surrounding code :)

// Many servers do not reliably send a close_notify alert when shutting
// down a connection, and instead terminate the TCP connection. This
// is reported as ERR_CONNECTION_CLOSED. Because of this, map the
// unclean shutdown to a graceful EOF, instead of treating it as an
// error as it should be.

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1500: result = ERR_CONNECTION_CLOSED;
On 2014/10/14 20:31:06, David Benjamin wrote:
> It is a little annoying that this fix is undoing this canonicalization. The
> awkward thing is that we /do/ want ERR_CONNECTION_CLOSED if it shuts off
during
> the handshake, but we /don't/ want it if it shuts off during a read.
> 
> Another option would be to:
> 
> - Remove this mapping. Instead, transport_read_error_ now stores 0 for EOF and
> we pick some other value (ERR_IO_PENDING?) to signal that it's still readable.
> Probably want to do the same thing to transport_write_error_. Then arrange for
> BIO_read of the SSL* end of transport_bio_ to return 0 if the underlying
> transport was closed, and -1 + OpenSSLPutNetError on error. This could either
by
> done by calling SSL_shutdown_wr or adjusting the callback. Either way,
> transport_read_error_ needs to be adjusted for the BufferRecv() guard in line
> 879.
> 

Sorry, I have trouble following you. It might be easier to see in code, but it
sounds... hackier?

> - This would make the SSL_read code DTRT because we'd get a zero return.
> SSL_get_error would return SSL_ERROR_ZERO_RETURN or SSL_ERROR_SYSCALL if we
> cared to distinguish close_notify but we don't.

DTRT?

> 
> - However, getting SSL_do_handshake right becomes tricky. (Before the original
> CL, we didn't bother because everthing turned into ERR_SSL_PROTOCOL_ERROR.)
> OpenSSL just bubbles the BIO return code up if it's 0 or -1. BUT it also
returns
> 0 if you get a close_notify during the handshake, which should be
> ERR_SSL_PROTOCOL_ERROR. The distinguishing factor is one gives
> SSL_ERROR_ZERO_RETURN in SSL_get_error and the other SSL_ERROR_SYSCALL. So I
> believe we need to first SSL_get_error, special-case rv == 0 && err ==
> SSL_ERROR_SYSCALL to ERR_CONNECTION_CLOSED, and then do the normal thing.
> 
> This is all kind of a mess and it seems plumbing EOF through the error
codepath
> and mapping back in Read is simpler, but I can do the other one if you prefer.

[davidben, 2014-10-20 17:13:24]

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

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1363: // ERR_CONNECTION_CLOSED cannot be
treated as fatal.
On 2014/10/17 20:53:00, Ryan Sleevi wrote:
> This comment needs explaining (or my flu meds need to not be as strong).
Namely,
> you're relying on a lot of internal context in your head for this comment that
> is lost in the surrounding code :)
> 
> // Many servers do not reliably send a close_notify alert when shutting
> // down a connection, and instead terminate the TCP connection. This
> // is reported as ERR_CONNECTION_CLOSED. Because of this, map the
> // unclean shutdown to a graceful EOF, instead of treating it as an
> // error as it should be.

Done.

https://codereview.chromium.org/655813003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1500: result = ERR_CONNECTION_CLOSED;
On 2014/10/17 20:53:00, Ryan Sleevi wrote:
> On 2014/10/14 20:31:06, David Benjamin wrote:
> > It is a little annoying that this fix is undoing this canonicalization. The
> > awkward thing is that we /do/ want ERR_CONNECTION_CLOSED if it shuts off
> during
> > the handshake, but we /don't/ want it if it shuts off during a read.
> > 
> > Another option would be to:
> > 
> > - Remove this mapping. Instead, transport_read_error_ now stores 0 for EOF
and
> > we pick some other value (ERR_IO_PENDING?) to signal that it's still
readable.
> > Probably want to do the same thing to transport_write_error_. Then arrange
for
> > BIO_read of the SSL* end of transport_bio_ to return 0 if the underlying
> > transport was closed, and -1 + OpenSSLPutNetError on error. This could
either
> by
> > done by calling SSL_shutdown_wr or adjusting the callback. Either way,
> > transport_read_error_ needs to be adjusted for the BufferRecv() guard in
line
> > 879.
> > 
> 
> Sorry, I have trouble following you. It might be easier to see in code, but it
> sounds... hackier?

I think I agree.

> > - This would make the SSL_read code DTRT because we'd get a zero return.
> > SSL_get_error would return SSL_ERROR_ZERO_RETURN or SSL_ERROR_SYSCALL if we
> > cared to distinguish close_notify but we don't.
> 
> DTRT?

Do The Right Thing. Sorry, I keep forgetting which acronyms are MIT slang and
which are Google slang. :-)

> > 
> > - However, getting SSL_do_handshake right becomes tricky. (Before the
original
> > CL, we didn't bother because everthing turned into ERR_SSL_PROTOCOL_ERROR.)
> > OpenSSL just bubbles the BIO return code up if it's 0 or -1. BUT it also
> returns
> > 0 if you get a close_notify during the handshake, which should be
> > ERR_SSL_PROTOCOL_ERROR. The distinguishing factor is one gives
> > SSL_ERROR_ZERO_RETURN in SSL_get_error and the other SSL_ERROR_SYSCALL. So I
> > believe we need to first SSL_get_error, special-case rv == 0 && err ==
> > SSL_ERROR_SYSCALL to ERR_CONNECTION_CLOSED, and then do the normal thing.
> > 
> > This is all kind of a mess and it seems plumbing EOF through the error
> codepath
> > and mapping back in Read is simpler, but I can do the other one if you
prefer.
>

[Ryan Sleevi, 2014-10-20 19:01:32]

lgtm

[davidben, 2014-10-20 19:02:32]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-10-20 19:03:43]

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

[commit-bot: I haz the power, 2014-10-20 19:16:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-20 19:17:03]

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