Include better OpenSSL error information in NetLog.

Include better OpenSSL error information in NetLog.

The 'ssl_lib_error' value reported is rather uninteresting. OpenSSL is has two
levels of errors. The one we were logging is almost always 1. Make the error
callback SSL-implementation-specific and include the library code, reason code,
file name, and line number, all of which OpenSSL already tracks.

BUG=286480
Committed: https://crrev.com/a4409c633c6a0ffe91df8098705f99dbec4c4f5f
Cr-Commit-Position: refs/heads/master@{#292173}

[davidben, 2014-08-20 23:10:50]

This stuff is basically free, so let's just report it all. This gives output
that looks like:

t=92827 [st=   2]     +SSL_CONNECT  [dt=4102]
t=92828 [st=   3]        SOCKET_BYTES_SENT
                         --> byte_count = 237
t=96929 [st=4104]        SOCKET_BYTES_RECEIVED
                         --> byte_count = 72
t=96929 [st=4104]        SSL_HANDSHAKE_ERROR
                         --> error_lib = 16
                         --> error_reason = 224
                         --> file =
"../../third_party/boringssl/src/ssl/s23_clnt.c"
                         --> line = 575
                         --> net_error = -107 (ERR_SSL_PROTOCOL_ERROR)
                         --> ssl_error = 1
t=96929 [st=4104]     -SSL_CONNECT

[Ryan Sleevi, 2014-08-25 06:24:35]

API wise, I think you're better off embracing a bit more C++-ishness here, as
evidenced by the number of times you have to initialize all three variables (all
of which are required)

https://codereview.chromium.org/494913002/diff/1/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/494913002/diff/1/net/socket/nss_ssl_util.cc#n...
net/socket/nss_ssl_util.cc:89: return dict;
Should you also call PR_GetErrorText  (noting PR_GetErrorTextLength) and
PR_GetOSError ?

Note: All of these have to be called on the originating error thread (they're in
the thread's thread local storage), which means you'd need to dupe the string
for safety

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

https://codereview.chromium.org/494913002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:915: ssl_error, err_tracer, &error_code,
&file, &line);
Rather than having to create the three local variables and trade them, does it
make more sense to marshal this into a simple struct and bind the callback to
that struct?

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

https://codereview.chromium.org/494913002/diff/1/net/ssl/openssl_ssl_util.cc#...
net/ssl/openssl_ssl_util.cc:172: dict->SetInteger("error_reason",
ERR_GET_REASON(error_code));
And we're guaranteed these will always be masks and never reach into something
like thread-local storage, right?

[davidben, 2014-08-26 22:13:51]

https://codereview.chromium.org/494913002/diff/1/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/494913002/diff/1/net/socket/nss_ssl_util.cc#n...
net/socket/nss_ssl_util.cc:89: return dict;
On 2014/08/25 06:24:35, Ryan Sleevi wrote:
> Should you also call PR_GetErrorText  (noting PR_GetErrorTextLength) and
> PR_GetOSError ?
> 
> Note: All of these have to be called on the originating error thread (they're
in
> the thread's thread local storage), which means you'd need to dupe the string
> for safety

Eh, this just came from net/socket/ssl_error_params.cc. I figured I'd just leave
it as-is so as not to have to think about these thread-local problems. :-)

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

https://codereview.chromium.org/494913002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:915: ssl_error, err_tracer, &error_code,
&file, &line);
On 2014/08/25 06:24:35, Ryan Sleevi wrote:
> Rather than having to create the three local variables and trade them, does it
> make more sense to marshal this into a simple struct and bind the callback to
> that struct?

Done.

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

https://codereview.chromium.org/494913002/diff/1/net/ssl/openssl_ssl_util.cc#...
net/ssl/openssl_ssl_util.cc:172: dict->SetInteger("error_reason",
ERR_GET_REASON(error_code));
On 2014/08/25 06:24:35, Ryan Sleevi wrote:
> And we're guaranteed these will always be masks and never reach into something
> like thread-local storage, right?

Yeah, that would be a pretty significant departure from how the API currently
works. I don't think you'd be able to make them not masks without serious hacks.
It needs to work with both ERR_get_error() and ERR_peek_error(), and the former
removes the error from the stack.

For more fun, there are fields that do come from per-thread storage, but you
extract them with a different API. Hence file/line being separate.

(I do actually want to eventually move ERR_GET_FUNC to per-thread storage
because the function codes are crazy. But I'm intentionally not calling that one
and the file/line information includes all that.)

[Ryan Sleevi, 2014-08-26 22:38:02]

lgtm

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

https://codereview.chromium.org/494913002/diff/20001/net/ssl/openssl_ssl_util...
net/ssl/openssl_ssl_util.h:44: OpenSSLErrorInfo();
An inline ctor is fine here, it's all POD, and no warning/yelling from clang. Up
to you though.

[davidben, 2014-08-27 15:48:22]

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

https://codereview.chromium.org/494913002/diff/20001/net/ssl/openssl_ssl_util...
net/ssl/openssl_ssl_util.h:44: OpenSSLErrorInfo();
On 2014/08/26 22:38:01, Ryan Sleevi wrote:
> An inline ctor is fine here, it's all POD, and no warning/yelling from clang.
Up
> to you though.

Done. Also set *out_error_info = OpenSSLErrorInfo() at the top of
MapOpenSSLErrorWithDetails because I apparently made the docs a lie.

[davidben, 2014-08-27 15:48:29]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-08-27 15:49:41]

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

[commit-bot: I haz the power, 2014-08-27 17:06:01]

Committed patchset #3 (id:40001) as 883822031ffc9f5e02304def921149f8b733530d

[commit-bot: I haz the power, 2014-09-10 02:52:23]

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