Skip to content
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

extras: Add meaningful description to BoringSSL errors #237

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

When an error occurs in a BoringSSL call, we typically embed the packed error code in a CryptoKitError.underlyingCoreCryptoError(error: Int32). This packed error code consists of the library and the reason and BoringSSL has functions for unpacking these and getting descriptive messages for them. I often spend my time in a debugger, manually calling these C functions and then looking things up in header files.

Modifications

In _CryptoExtras (not in Crypto) add a (possibly retroactive) conformance to CustomStringConvertible.

Result

If _CryptoExtras is imported, errors will likely be much more informative. For example,

error: "underlyingCoreCryptoError(error: 50331755)"  // before

error: "lib: bignum routines, reason: INPUT_NOT_REDUCED, code: 50331755"  // after

@Lukasa
Copy link
Contributor

Lukasa commented Jun 6, 2024

I'm not wild about adding a retroactive conformance: we don't have a unique privilege to add this conformance. Can I suggest instead we add a different overload without a conformance?

@simonjbeaumont
Copy link
Contributor Author

So, I've had this kicking around on my machine as an executable target, called crypto-errno. Do you think that'd be a better way to have this information provided for development and, if so, worth having in the repo?

@Lukasa
Copy link
Contributor

Lukasa commented Sep 30, 2024

We could do both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants