-
Notifications
You must be signed in to change notification settings - Fork 907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More detailed OpenSSL errors #1435
base: master
Are you sure you want to change the base?
Conversation
Fixed the MSVC/Win2016 build failures, but I don't think this PR caused the most recent MacOS build failure. |
const char *extra_suffix = has_data ? "]" : ""; | ||
const char *extra = has_data ? data : ""; | ||
|
||
lines.add(kj::str("[", fname, ":", lno, "] ", message, extra_prefix, extra, extra_suffix)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it fname
is the OpenSSL source file name?
kj::Exception
itself already stores a filename and line number, maybe we should initialize that directly? Like so:
kj::throwFatalException(kj::Exception(kj::Exception::Type::FAILED,
fname, lno, kj::str(message, extra_prefix, extra, extra_suffix));
The one problem is how to deal with multiple errors. In that case I'd say we want the overall exception's source location to match either the first or last error (not sure which is more appropriate...) and then the rest could be in the description... maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it fname is the OpenSSL source file name?
That's correct, yep. Sometimes it's a weird relative path like ../ssl/ssl_lib.c
, seems to depend on how OpenSSL was built.
The one problem is how to deal with multiple errors. In that case I'd say we want the overall exception's source location to match either the first or last error (not sure which is more appropriate...) and then the rest could be in the description... maybe?
Ah I think the original semantics make sense, like I'd expect the filename + lineno to match the exception "callsite" myself. If including filename + line number info from OpenSSL in the error message makes you uneasy I'm not particularly attached to it. The data
fragment is more important IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the exception's stack trace will already identify the location where it is constructed, so identifying the location of the KJ assertion is actually redundant.
To me, what we're doing here is translating an openssl exception to a KJ exception, and it feels right for the filename / line number to be part of that translation.
But on a practical note, I guess the OpenSSL source location probably isn't that useful to anyone who isn't an OpenSSL developer, so maybe that's a good argument for just omitting it?
c35c724
to
6ca3362
Compare
This may need to merge in latest changes ( like PR #1596 ) |
While working on the OpenSSL 3.0 test failures that led to #1434 I ran into a test failure that was a bit harder to debug because there wasn't a lot of detail in the OpenSSL error reported by capnproto:
Here the
system library
is reporting a missing file/directory, but which one?It turns out the details are hidden away in the
data
associated with the error. The same error with additional detail fromERR_get_error_all
(the OpenSSL 3.x function that supersedesERR_get_error_line_data
) from an earlier incarnation of this change:We can clearly see OpenSSL was looking for
/usr/lib/ssl/certs
but it didn't exist. With this info in the build logs it was trivial to fix the broken test.To make this kind of issue clearer/easier to debug going forward this PR outputs the additional error detail (filename, lineno, data) as part of
throwOpensslError
.Note that it's tricky to force an error with
data
set so the test here is a little incomplete, but the implementation follows examples found in the OpenSSL code base.