rustls / rustls-ffi

Use Rustls from any language
Other
128 stars 30 forks source link

allow getting more information out of errors #375

Open jsha opened 10 months ago

jsha commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.