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

allow getting more information out of errors #375

Open
jsha opened this issue Dec 5, 2023 · 2 comments
Open

allow getting more information out of errors #375

jsha opened this issue Dec 5, 2023 · 2 comments

Comments

@jsha
Copy link
Collaborator

jsha commented Dec 5, 2023

This library treats all error as mappable into one big enum, mapping onto the variants of rustls::Error, with some variants exploded into multiple values (like InvalidMessage) and some elided like the Vec in InappropriateMessage and InappropriateHandshakeMessage.

One place where this falls down is the General error type, where we elide the string value. That means RUSTLS_RESULT_GENERAL loses some information. But historically there weren't many paths that returned General so this wasn't a big deal.

In rustls 0.22 there's the new OtherError variant, which passes through an arbitrary error from a cryptographic backend. This has the same problem - right now we turn it into RUSTLS_RESULT_GENERAL, which loses information.

Returning an error enum (result a u32) is very handy because there is no allocation involved. The caller can discard the value without having to worry about freeing it.

However, we should consider changing the whole error structure. Instead of returning an enum, we could return a pointer to an opaque type *rustls_err. Returning a null pointer would indicate success, while returning a non-null pointer would indicate error. There would be a method on *rustls_err to extract the string value of the error, and another method to get the top-level enum variant.

This has the downside that the caller needs to free the error once they are done processing it. However, this may not be a huge burden because C error handling flows often include a goto cleanup where any non-NULL pointers that may have been allocated during the function get freed.

@cpu
Copy link
Member

cpu commented Dec 6, 2023

It might be interesting to do a comparison with some other large C APIs to see if any handle errors similarly. I'm not enough of a C developer to be able to decide without more research if this is an especially unusual approach or the logical conclusion for wanting to move beyond simple error codes.

@ctz Maybe you have some thoughts as someone that's done more C coding?

@ctz
Copy link
Member

ctz commented Dec 6, 2023

Memory allocations that only happen in error branches seems like a recipe for memory leaks (though OpenSSL does that, it hangs on to the allocations in a global and lets you clear it from anywhere via ERR_clear_error() -- I bet you a million there are lots of latent memory leaks as a result!)

I think my preferred option would for a win32-style "get last error" API, attached to each object that you could get an error from. rustls_result could continue to be an enum, but callers wanting additional information could call eg. rustls_result rustls_acceptor_last_error(struct rustls_acceptor *acceptor, rustls_str *error_string_out) or something.

The underlying stored error can be cleared up with the owning object (eg, rustls_acceptor_free), which is presumably happening correctly already.

Another -- simpler and less satisfying -- option would be to log every error detail that we throw away when converting to rustls_result.

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

No branches or pull requests

3 participants