From bcec9462aff4ba7fd11d508ee8469bf66abf5168 Mon Sep 17 00:00:00 2001 From: Kornel Date: Thu, 5 Jun 2025 02:15:42 +0100 Subject: [PATCH] Don't unwrap when Result can be returned instead --- boring/src/asn1.rs | 4 +-- boring/src/bn.rs | 4 +-- boring/src/error.rs | 55 ++++++++++++++++++++++++++++++++++++++++-- boring/src/nid.rs | 8 +++--- boring/src/pkcs12.rs | 6 ++--- boring/src/pkey.rs | 2 +- boring/src/ssl/mod.rs | 24 ++++++++++-------- boring/src/x509/mod.rs | 13 +++++----- 8 files changed, 86 insertions(+), 30 deletions(-) diff --git a/boring/src/asn1.rs b/boring/src/asn1.rs index 9fb3fcaf..ceb76c0c 100644 --- a/boring/src/asn1.rs +++ b/boring/src/asn1.rs @@ -323,7 +323,7 @@ impl Asn1Time { #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> Result { unsafe { - let s = CString::new(s).unwrap(); + let s = CString::new(s).map_err(ErrorStack::internal_error)?; let time = Asn1Time::new()?; cvt(ffi::ASN1_TIME_set_string(time.as_ptr(), s.as_ptr()))?; @@ -560,7 +560,7 @@ impl Asn1Object { pub fn from_str(txt: &str) -> Result { unsafe { ffi::init(); - let txt = CString::new(txt).unwrap(); + let txt = CString::new(txt).map_err(ErrorStack::internal_error)?; let obj: *mut ffi::ASN1_OBJECT = cvt_p(ffi::OBJ_txt2obj(txt.as_ptr() as *const _, 0))?; Ok(Asn1Object::from_ptr(obj)) } diff --git a/boring/src/bn.rs b/boring/src/bn.rs index 58edbabb..f6773a95 100644 --- a/boring/src/bn.rs +++ b/boring/src/bn.rs @@ -838,7 +838,7 @@ impl BigNum { pub fn from_dec_str(s: &str) -> Result { unsafe { ffi::init(); - let c_str = CString::new(s.as_bytes()).unwrap(); + let c_str = CString::new(s.as_bytes()).map_err(ErrorStack::internal_error)?; let mut bn = ptr::null_mut(); cvt(ffi::BN_dec2bn(&mut bn, c_str.as_ptr() as *const _))?; Ok(BigNum::from_ptr(bn)) @@ -850,7 +850,7 @@ impl BigNum { pub fn from_hex_str(s: &str) -> Result { unsafe { ffi::init(); - let c_str = CString::new(s.as_bytes()).unwrap(); + let c_str = CString::new(s.as_bytes()).map_err(ErrorStack::internal_error)?; let mut bn = ptr::null_mut(); cvt(ffi::BN_hex2bn(&mut bn, c_str.as_ptr() as *const _))?; Ok(BigNum::from_ptr(bn)) diff --git a/boring/src/error.rs b/boring/src/error.rs index dd39e94e..5d3e61fe 100644 --- a/boring/src/error.rs +++ b/boring/src/error.rs @@ -48,6 +48,12 @@ impl ErrorStack { error.put(); } } + + /// Used to report errors from the Rust crate + #[cold] + pub(crate) fn internal_error(err: impl error::Error) -> Self { + Self(vec![Error::new_internal(err.to_string())]) + } } impl ErrorStack { @@ -69,7 +75,11 @@ impl fmt::Display for ErrorStack { fmt.write_str(" ")?; } first = false; - write!(fmt, "[{}]", err.reason().unwrap_or("unknown reason"))?; + write!( + fmt, + "[{}]", + err.reason_internal().unwrap_or("unknown reason") + )?; } Ok(()) } @@ -101,6 +111,8 @@ pub struct Error { unsafe impl Sync for Error {} unsafe impl Send for Error {} +static BORING_INTERNAL: &CStr = c"boring-rust"; + impl Error { /// Returns the first error on the OpenSSL error stack. pub fn get() -> Option { @@ -171,6 +183,9 @@ impl Error { /// Returns the name of the library reporting the error, if available. pub fn library(&self) -> Option<&'static str> { + if self.is_internal() { + return None; + } unsafe { let cstr = ffi::ERR_lib_error_string(self.code); if cstr.is_null() { @@ -189,6 +204,9 @@ impl Error { /// Returns the name of the function reporting the error. pub fn function(&self) -> Option<&'static str> { + if self.is_internal() { + return None; + } unsafe { let cstr = ffi::ERR_func_error_string(self.code); if cstr.is_null() { @@ -237,6 +255,28 @@ impl Error { pub fn data(&self) -> Option<&str> { self.data.as_deref() } + + fn new_internal(msg: String) -> Self { + Self { + code: ffi::ERR_PACK(ffi::ERR_LIB_NONE.0 as _, 0, 0) as _, + file: BORING_INTERNAL.as_ptr(), + line: 0, + data: Some(msg.into()), + } + } + + fn is_internal(&self) -> bool { + std::ptr::eq(self.file, BORING_INTERNAL.as_ptr()) + } + + // reason() needs 'static + fn reason_internal(&self) -> Option<&str> { + if self.is_internal() { + self.data() + } else { + self.reason() + } + } } impl fmt::Debug for Error { @@ -268,7 +308,7 @@ impl fmt::Display for Error { write!( fmt, "{}\n\nCode: {:08X}\nLoc: {}:{}", - self.reason().unwrap_or("unknown TLS error"), + self.reason_internal().unwrap_or("unknown TLS error"), self.code(), self.file(), self.line() @@ -277,3 +317,14 @@ impl fmt::Display for Error { } impl error::Error for Error {} + +#[test] +fn internal_err() { + let e = ErrorStack::internal_error(io::Error::other("hello, boring")); + assert_eq!(1, e.errors().len()); + assert!(e.to_string().contains("hello, boring"), "{e} {e:?}"); + + e.put(); + let e = ErrorStack::get(); + assert!(e.to_string().contains("hello, boring"), "{e} {e:?}"); +} diff --git a/boring/src/nid.rs b/boring/src/nid.rs index 11607626..f2243723 100644 --- a/boring/src/nid.rs +++ b/boring/src/nid.rs @@ -84,8 +84,8 @@ impl Nid { #[allow(clippy::trivially_copy_pass_by_ref)] pub fn long_name(&self) -> Result<&'static str, ErrorStack> { unsafe { - cvt_p(ffi::OBJ_nid2ln(self.0) as *mut c_char) - .map(|nameptr| str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).unwrap()) + let nameptr = cvt_p(ffi::OBJ_nid2ln(self.0) as *mut c_char)?; + str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).map_err(ErrorStack::internal_error) } } @@ -94,8 +94,8 @@ impl Nid { #[allow(clippy::trivially_copy_pass_by_ref)] pub fn short_name(&self) -> Result<&'static str, ErrorStack> { unsafe { - cvt_p(ffi::OBJ_nid2sn(self.0) as *mut c_char) - .map(|nameptr| str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).unwrap()) + let nameptr = cvt_p(ffi::OBJ_nid2sn(self.0) as *mut c_char)?; + str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).map_err(ErrorStack::internal_error) } } diff --git a/boring/src/pkcs12.rs b/boring/src/pkcs12.rs index 8604f6d1..72b40906 100644 --- a/boring/src/pkcs12.rs +++ b/boring/src/pkcs12.rs @@ -34,7 +34,7 @@ impl Pkcs12Ref { /// Extracts the contents of the `Pkcs12`. pub fn parse(&self, pass: &str) -> Result { unsafe { - let pass = CString::new(pass.as_bytes()).unwrap(); + let pass = CString::new(pass.as_bytes()).map_err(ErrorStack::internal_error)?; let mut pkey = ptr::null_mut(); let mut cert = ptr::null_mut(); @@ -161,8 +161,8 @@ impl Pkcs12Builder { T: HasPrivate, { unsafe { - let pass = CString::new(password).unwrap(); - let friendly_name = CString::new(friendly_name).unwrap(); + let pass = CString::new(password).map_err(ErrorStack::internal_error)?; + let friendly_name = CString::new(friendly_name).map_err(ErrorStack::internal_error)?; let pkey = pkey.as_ptr(); let cert = cert.as_ptr(); let ca = self diff --git a/boring/src/pkey.rs b/boring/src/pkey.rs index 1c4012ca..fc9a78e2 100644 --- a/boring/src/pkey.rs +++ b/boring/src/pkey.rs @@ -408,7 +408,7 @@ impl PKey { unsafe { ffi::init(); let bio = MemBioSlice::new(der)?; - let passphrase = CString::new(passphrase).unwrap(); + let passphrase = CString::new(passphrase).map_err(ErrorStack::internal_error)?; cvt_p(ffi::d2i_PKCS8PrivateKey_bio( bio.as_ptr(), ptr::null_mut(), diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index a03cdf6e..c293ed00 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -1269,7 +1269,8 @@ impl SslContextBuilder { #[cfg(feature = "rpk")] assert!(!self.is_rpk, "This API is not supported for RPK"); - let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); + let file = CString::new(file.as_ref().as_os_str().as_encoded_bytes()) + .map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_CTX_load_verify_locations( self.as_ptr(), @@ -1340,7 +1341,8 @@ impl SslContextBuilder { #[cfg(feature = "rpk")] assert!(!self.is_rpk, "This API is not supported for RPK"); - let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); + let file = CString::new(file.as_ref().as_os_str().as_encoded_bytes()) + .map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_CTX_use_certificate_file( self.as_ptr(), @@ -1361,7 +1363,8 @@ impl SslContextBuilder { &mut self, file: P, ) -> Result<(), ErrorStack> { - let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); + let file = CString::new(file.as_ref().as_os_str().as_encoded_bytes()) + .map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_CTX_use_certificate_chain_file( self.as_ptr(), @@ -1401,7 +1404,8 @@ impl SslContextBuilder { file: P, file_type: SslFiletype, ) -> Result<(), ErrorStack> { - let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); + let file = CString::new(file.as_ref().as_os_str().as_encoded_bytes()) + .map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_CTX_use_PrivateKey_file( self.as_ptr(), @@ -1564,7 +1568,7 @@ impl SslContextBuilder { #[corresponds(SSL_CTX_set_tlsext_use_srtp)] pub fn set_tlsext_use_srtp(&mut self, protocols: &str) -> Result<(), ErrorStack> { unsafe { - let cstr = CString::new(protocols).unwrap(); + let cstr = CString::new(protocols).map_err(ErrorStack::internal_error)?; let r = ffi::SSL_CTX_set_tlsext_use_srtp(self.as_ptr(), cstr.as_ptr()); // fun fact, set_tlsext_use_srtp has a reversed return code D: @@ -1908,7 +1912,7 @@ impl SslContextBuilder { /// Sets the context's supported signature algorithms. #[corresponds(SSL_CTX_set1_sigalgs_list)] pub fn set_sigalgs_list(&mut self, sigalgs: &str) -> Result<(), ErrorStack> { - let sigalgs = CString::new(sigalgs).unwrap(); + let sigalgs = CString::new(sigalgs).map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_CTX_set1_sigalgs_list(self.as_ptr(), sigalgs.as_ptr()) as c_int) .map(|_| ()) @@ -1968,7 +1972,7 @@ impl SslContextBuilder { #[cfg(not(feature = "kx-safe-default"))] #[corresponds(SSL_CTX_set1_curves_list)] pub fn set_curves_list(&mut self, curves: &str) -> Result<(), ErrorStack> { - let curves = CString::new(curves).unwrap(); + let curves = CString::new(curves).map_err(ErrorStack::internal_error)?; unsafe { cvt_0i(ffi::SSL_CTX_set1_curves_list( self.as_ptr(), @@ -2802,7 +2806,7 @@ impl SslRef { #[corresponds(SSL_set1_curves_list)] pub fn set_curves_list(&mut self, curves: &str) -> Result<(), ErrorStack> { - let curves = CString::new(curves).unwrap(); + let curves = CString::new(curves).map_err(ErrorStack::internal_error)?; unsafe { cvt_0i(ffi::SSL_set1_curves_list( self.as_ptr(), @@ -3086,7 +3090,7 @@ impl SslRef { /// It has no effect for a server-side connection. #[corresponds(SSL_set_tlsext_host_name)] pub fn set_hostname(&mut self, hostname: &str) -> Result<(), ErrorStack> { - let cstr = CString::new(hostname).unwrap(); + let cstr = CString::new(hostname).map_err(ErrorStack::internal_error)?; unsafe { cvt(ffi::SSL_set_tlsext_host_name(self.as_ptr(), cstr.as_ptr() as *mut _) as c_int) .map(|_| ()) @@ -3273,7 +3277,7 @@ impl SslRef { #[corresponds(SSL_set_tlsext_use_srtp)] pub fn set_tlsext_use_srtp(&mut self, protocols: &str) -> Result<(), ErrorStack> { unsafe { - let cstr = CString::new(protocols).unwrap(); + let cstr = CString::new(protocols).map_err(ErrorStack::internal_error)?; let r = ffi::SSL_set_tlsext_use_srtp(self.as_ptr(), cstr.as_ptr()); // fun fact, set_tlsext_use_srtp has a reversed return code D: diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index e9b2937c..2e17dbb3 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -894,8 +894,8 @@ impl X509Extension { name: &str, value: &str, ) -> Result { - let name = CString::new(name).unwrap(); - let value = CString::new(value).unwrap(); + let name = CString::new(name).map_err(ErrorStack::internal_error)?; + let value = CString::new(value).map_err(ErrorStack::internal_error)?; let mut ctx; unsafe { ffi::init(); @@ -940,7 +940,7 @@ impl X509Extension { name: Nid, value: &str, ) -> Result { - let value = CString::new(value).unwrap(); + let value = CString::new(value).map_err(ErrorStack::internal_error)?; let mut ctx; unsafe { ffi::init(); @@ -1004,7 +1004,7 @@ impl X509NameBuilder { #[corresponds(X509_NAME_add_entry_by_txt)] pub fn append_entry_by_text(&mut self, field: &str, value: &str) -> Result<(), ErrorStack> { unsafe { - let field = CString::new(field).unwrap(); + let field = CString::new(field).map_err(ErrorStack::internal_error)?; assert!(value.len() <= ValueLen::MAX as usize); cvt(ffi::X509_NAME_add_entry_by_txt( self.0.as_ptr(), @@ -1028,7 +1028,7 @@ impl X509NameBuilder { ty: Asn1Type, ) -> Result<(), ErrorStack> { unsafe { - let field = CString::new(field).unwrap(); + let field = CString::new(field).map_err(ErrorStack::internal_error)?; assert!(value.len() <= ValueLen::MAX as usize); cvt(ffi::X509_NAME_add_entry_by_txt( self.0.as_ptr(), @@ -1116,7 +1116,8 @@ impl X509Name { /// /// This is commonly used in conjunction with `SslContextBuilder::set_client_ca_list`. pub fn load_client_ca_file>(file: P) -> Result, ErrorStack> { - let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); + let file = CString::new(file.as_ref().as_os_str().as_encoded_bytes()) + .map_err(ErrorStack::internal_error)?; unsafe { cvt_p(ffi::SSL_load_client_CA_file(file.as_ptr())).map(|p| Stack::from_ptr(p)) } }