-
Notifications
You must be signed in to change notification settings - Fork 30k
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
crypto: use BoringSSL compatible errors #37297
crypto: use BoringSSL compatible errors #37297
Conversation
60485ad
to
f345f5d
Compare
Is electron currently using this exact patch?
I think that's been a goal for a long time, but it isn't always simple. For example, when we do explicitly throw OpenSSL errors, we often do so to align with other APIs that might throw the exact same errors, except that they originate within OpenSSL and not Node.js. Changing the error codes to something else would not only be semver-major, but might also introduce inconsistencies with errors originating within OpenSSL. |
@tniessen we patch it to be e.g.
which is the BoringSSL macro that maps to what y'all have now - this change is the common ancestor of both |
@codebytere Thanks! Are all of these constants available in BoringSSL, or would this patch still cause compilation issues when linking against BoringSSL? I remember that some error codes didn't exist in BoringSSL when we ported some other API. |
@tniessen yes! this is cross-compatible in its current state :) |
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.
Thanks @codebytere :)
Landed in 8353854 |
PR-URL: #37297 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #37297 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This PR migrates OpenSSL-specific error messages (
BNerr
&Dherr
) to the underlying error calls, which BoringSSL is also capable of invoking. Electron is currently patching these out, and we'd prefer not to.Another consideration we might perhaps make, however, is instead just throwing more prototypical Node.js errors - curious what y'all think.