From d333fdb92d99b36966837f53a07c566b6e7138b6 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 4 Nov 2021 10:18:22 -0700 Subject: [PATCH 1/2] tokio-boring: Add additional accessors to `HandshakeError` Currently, the `HandshakeError` type in `tokio-boring` provides a `std::error::Error` implementation and an `as_io_error` method that returns an `Option<&std::io::Error>`, as the only ways to access the potentially underlying error. There are a couple of cases where this is somewhat insufficient: * If a user has an owned `HandshakeError`, and needs an owned `io::Error`, they have to create a *new* one, using the `HandshakeError`'s cause and message, like this: ```rust let err: HandshakeError<_> = // ...; if let Some(io_err) = err.as_io_error() { return Err(io::Error::new(io_err.cause(), io_err.to_string())); } // ... ``` This seems like kind of a shame, since it allocates two additional times (for the `String` and then again for the `io::Error` itself), and deallocates the original `io::Error` at the end of the scope. None of this *should* be necessary, since the `HandshakeError` in this case is owned and the original `io::Error` could be returned. * `HandshakeError` only implements `fmt::Debug` and `fmt::Display` when the wrapped I/O stream it's generic over implements `Debug`. This means that bounding the `S` type with `Debug` is necessary for `HandshakeError` to implement the `Error` trait. In some cases, introducing a `Debug` bound on a generic I/O type is kind of a heavyweight requirement. Therefore, it would be nice to have a way to extract a type that *always* implements `Error` from the `HandshakeError`, in cases where the returned I/O stream is just going to be discarded if the handshake failed. This branch introduces new functions on `HandshakeError`: * `HandshakeError::as_ssl_error`, which is analogous to `HandshakeError::as_io_error` but returning an `Option<&boring::error::ErrorStack>` instead of an I/O error. * `HandshakeError::into_io_error` and `HandshakeError::into_ssl_error`, which consume the error and return an `Option` or an `Option`, respectively. I noticed that some of the `into_$TYPE` methods on errors in the `boring` crate return `Result`s rather than `Option`s so that the original error can be reused if it couldn't be converted into the requested type. However, this can't easily be done in `HandshakeError`'s `into_io_error` and `into_ssl_error`, because these methods call `MidHandshakeSslStream::into_error`, and (since `MidHandshakeSslStream` can only be constructed by the `boring` crate, not in `tokio-boring`), we can't ever get the `MidHandshakeSslStream` _back_...so we can't return the original `Self` type in this case. Hopefully this should make it much easier to convert `HandshakeError`s into other error types as required in user code! Signed-off-by: Eliza Weisman --- tokio-boring/src/lib.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tokio-boring/src/lib.rs b/tokio-boring/src/lib.rs index c5c25044b..88d37bff8 100644 --- a/tokio-boring/src/lib.rs +++ b/tokio-boring/src/lib.rs @@ -307,6 +307,40 @@ impl HandshakeError { _ => None, } } + + /// Returns a reference to the inner SSL error, if this error was caused by + /// a SSL error. + pub fn as_ssl_error(&self) -> Option<&boring::error::ErrorStack> { + match &self.0 { + ssl::HandshakeError::Failure(s) => s.error().ssl_error(), + ssl::HandshakeError::SetupFailure(ref s) => Some(s), + _ => None, + } + } + + /// Consumes `self` and returns the inner I/O error by value, if this error + /// was caused by an I/O error. + pub fn into_io_error(self) -> Option { + match self.0 { + ssl::HandshakeError::Failure(s) => s.into_error().into_io_error().ok(), + _ => None, + } + } + + /// Consumes `self` and returns the inner SSL error by value, if this error + /// was caused by n SSL error. + pub fn into_ssl_error(self) -> Option { + match self.0 { + // TODO(eliza): it's not great that we have to clone in this case, + // but `boring::ssl::Error` doesn't currently expose an + // `into_ssl_error` the way it does for `io::Error`s. We could add + // that in a separate PR, but adding that here would make + // `tokio-boring` depend on a new release of `boring`... + ssl::HandshakeError::Failure(s) => s.into_error().ssl_error().cloned(), + ssl::HandshakeError::SetupFailure(stack) => Some(stack), + _ => None, + } + } } impl fmt::Debug for HandshakeError From 986a8ca6d970eaa7e04173a3e6fb4f7d1e436264 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 5 Nov 2021 10:24:47 -0700 Subject: [PATCH 2/2] return `Result`s from `into_$ERROR` methods Signed-off-by: Eliza Weisman --- tokio-boring/src/lib.rs | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/tokio-boring/src/lib.rs b/tokio-boring/src/lib.rs index 88d37bff8..21cb94e32 100644 --- a/tokio-boring/src/lib.rs +++ b/tokio-boring/src/lib.rs @@ -320,25 +320,43 @@ impl HandshakeError { /// Consumes `self` and returns the inner I/O error by value, if this error /// was caused by an I/O error. - pub fn into_io_error(self) -> Option { + /// + /// If this error was not caused by an I/O error, returns `Err(self)` so + /// that the error value can be reused. + pub fn into_io_error(self) -> Result { match self.0 { - ssl::HandshakeError::Failure(s) => s.into_error().into_io_error().ok(), - _ => None, + ssl::HandshakeError::Failure(s) => { + if s.error().io_error().is_some() { + Ok(s.into_error().into_io_error().expect( + "if `s.error().io_error().is_some()`, `into_io_error()` must succeed", + )) + } else { + Err(Self(ssl::HandshakeError::Failure(s))) + } + } + _ => Err(self), } } /// Consumes `self` and returns the inner SSL error by value, if this error - /// was caused by n SSL error. - pub fn into_ssl_error(self) -> Option { + /// was caused by a SSL error. + /// + /// If this error was not caused by a SSL error, returns `Err(self)` so + /// that the error value can be reused. + pub fn into_ssl_error(self) -> Result { match self.0 { // TODO(eliza): it's not great that we have to clone in this case, // but `boring::ssl::Error` doesn't currently expose an // `into_ssl_error` the way it does for `io::Error`s. We could add // that in a separate PR, but adding that here would make // `tokio-boring` depend on a new release of `boring`... - ssl::HandshakeError::Failure(s) => s.into_error().ssl_error().cloned(), - ssl::HandshakeError::SetupFailure(stack) => Some(stack), - _ => None, + ssl::HandshakeError::Failure(s) => s + .error() + .ssl_error() + .cloned() + .ok_or_else(|| Self(ssl::HandshakeError::Failure(s))), + ssl::HandshakeError::SetupFailure(stack) => Ok(stack), + _ => Err(self), } } }