From e95c3ef9cffcd6b4b607f7763931c9a50963b46f Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 1 Oct 2025 11:50:08 +0100 Subject: [PATCH 1/3] Fix string data conversion in ErrorStack::put() --- boring/src/error.rs | 56 +++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/boring/src/error.rs b/boring/src/error.rs index 60d8d4f5..5c1ad40b 100644 --- a/boring/src/error.rs +++ b/boring/src/error.rs @@ -20,6 +20,7 @@ use openssl_macros::corresponds; use std::borrow::Cow; use std::error; use std::ffi::CStr; +use std::ffi::CString; use std::fmt; use std::io; use std::ptr; @@ -58,7 +59,7 @@ impl ErrorStack { /// 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())]) + Self(vec![Error::new_internal(Data::String(err.to_string()))]) } /// Empties the current thread's error queue. @@ -122,7 +123,14 @@ pub struct Error { code: c_uint, file: *const c_char, line: c_uint, - data: Option>, + data: Data, +} + +#[derive(Clone)] +enum Data { + None, + CString(CString), + String(String), } unsafe impl Sync for Error {} @@ -148,11 +156,9 @@ impl Error { // The memory referenced by data is only valid until that slot is overwritten // in the error stack, so we'll need to copy it off if it's dynamic let data = if flags & ffi::ERR_FLAG_STRING != 0 { - Some(Cow::Owned( - CStr::from_ptr(data.cast()).to_string_lossy().into_owned(), - )) + Data::CString(CStr::from_ptr(data.cast()).to_owned()) } else { - None + Data::None }; Some(Error { code, @@ -176,22 +182,8 @@ impl Error { self.file, self.line, ); - let ptr = match self.data { - Some(Cow::Borrowed(data)) => Some(data.as_ptr() as *mut c_char), - Some(Cow::Owned(ref data)) => { - let ptr = ffi::OPENSSL_malloc((data.len() + 1) as _) as *mut c_char; - if ptr.is_null() { - None - } else { - ptr::copy_nonoverlapping(data.as_ptr(), ptr as *mut u8, data.len()); - *ptr.add(data.len()) = 0; - Some(ptr) - } - } - None => None, - }; - if let Some(ptr) = ptr { - ffi::ERR_add_error_data(1, ptr); + if let Some(cstr) = self.data_cstr() { + ffi::ERR_set_error_data(cstr.as_ptr().cast_mut(), ffi::ERR_FLAG_STRING); } } } @@ -297,15 +289,29 @@ impl Error { /// Returns additional data describing the error. #[must_use] pub fn data(&self) -> Option<&str> { - self.data.as_deref() + match &self.data { + Data::None => None, + Data::CString(cstring) => cstring.to_str().ok(), + Data::String(s) => Some(s), + } + } + + #[must_use] + fn data_cstr(&self) -> Option> { + let s = match &self.data { + Data::None => return None, + Data::CString(cstr) => return Some(Cow::Borrowed(cstr)), + Data::String(s) => s.as_str(), + }; + CString::new(s).ok().map(Cow::Owned) } - fn new_internal(msg: String) -> Self { + fn new_internal(msg: Data) -> 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()), + data: msg, } } From f052bee4f7b540c241f0f32b06b2fca3e64e8a55 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 1 Oct 2025 11:59:59 +0100 Subject: [PATCH 2/3] ErrorStack ctor for custom errors --- boring/src/error.rs | 12 ++++++++++++ boring/src/ssl/callbacks.rs | 2 +- boring/src/symm.rs | 4 ++-- boring/src/util.rs | 4 ++-- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/boring/src/error.rs b/boring/src/error.rs index 5c1ad40b..365643d6 100644 --- a/boring/src/error.rs +++ b/boring/src/error.rs @@ -38,6 +38,9 @@ pub struct ErrorStack(Vec); impl ErrorStack { /// Pops the contents of the OpenSSL error stack, and returns it. + /// + /// This should be used only immediately after calling Boring FFI functions, + /// otherwise the stack may be empty or a leftover from unrelated calls. #[corresponds(ERR_get_error_line_data)] #[must_use = "Use ErrorStack::clear() to drop the error stack"] pub fn get() -> ErrorStack { @@ -62,6 +65,12 @@ impl ErrorStack { Self(vec![Error::new_internal(Data::String(err.to_string()))]) } + /// Used to report errors from the Rust crate + #[cold] + pub(crate) fn internal_error_str(message: &'static str) -> Self { + Self(vec![Error::new_internal(Data::Static(message))]) + } + /// Empties the current thread's error queue. #[corresponds(ERR_clear_error)] pub(crate) fn clear() { @@ -131,6 +140,7 @@ enum Data { None, CString(CString), String(String), + Static(&'static str), } unsafe impl Sync for Error {} @@ -293,6 +303,7 @@ impl Error { Data::None => None, Data::CString(cstring) => cstring.to_str().ok(), Data::String(s) => Some(s), + Data::Static(s) => Some(s), } } @@ -302,6 +313,7 @@ impl Error { Data::None => return None, Data::CString(cstr) => return Some(Cow::Borrowed(cstr)), Data::String(s) => s.as_str(), + Data::Static(s) => s, }; CString::new(s).ok().map(Cow::Owned) } diff --git a/boring/src/ssl/callbacks.rs b/boring/src/ssl/callbacks.rs index ed724f79..ea0a73c2 100644 --- a/boring/src/ssl/callbacks.rs +++ b/boring/src/ssl/callbacks.rs @@ -767,7 +767,7 @@ impl<'a> CryptoBufferBuilder<'a> { let buffer_capacity = unsafe { ffi::CRYPTO_BUFFER_len(self.buffer) }; if self.cursor.position() != buffer_capacity as u64 { // Make sure all bytes in buffer initialized as required by Boring SSL. - return Err(ErrorStack::get()); + return Err(ErrorStack::internal_error_str("invalid len")); } unsafe { let mut result = ptr::null_mut(); diff --git a/boring/src/symm.rs b/boring/src/symm.rs index 38fab76d..a1346e6e 100644 --- a/boring/src/symm.rs +++ b/boring/src/symm.rs @@ -89,7 +89,7 @@ impl CipherCtxRef { ffi::init(); if key.len() != cipher.key_len() { - return Err(ErrorStack::get()); + return Err(ErrorStack::internal_error_str("invalid key size")); } unsafe { @@ -117,7 +117,7 @@ impl CipherCtxRef { ffi::init(); if key.len() != cipher.key_len() { - return Err(ErrorStack::get()); + return Err(ErrorStack::internal_error_str("invalid key size")); } unsafe { diff --git a/boring/src/util.rs b/boring/src/util.rs index bb6373c1..d34fd898 100644 --- a/boring/src/util.rs +++ b/boring/src/util.rs @@ -55,8 +55,8 @@ where match result { Ok(Ok(len)) => len as c_int, - Ok(Err(_)) => { - // FIXME restore error stack + Ok(Err(err)) => { + err.put(); 0 } Err(err) => { From f9900f741cb3f2f4051a9d31eb3fb3e0d2a4861e Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 1 Oct 2025 11:12:40 +0100 Subject: [PATCH 3/3] Safer CryptoBufferBuilder::build --- boring/src/ssl/callbacks.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/boring/src/ssl/callbacks.rs b/boring/src/ssl/callbacks.rs index ea0a73c2..f08409b3 100644 --- a/boring/src/ssl/callbacks.rs +++ b/boring/src/ssl/callbacks.rs @@ -16,7 +16,7 @@ use foreign_types::ForeignType; use foreign_types::ForeignTypeRef; use libc::{c_char, c_int, c_uchar, c_uint, c_void}; use std::ffi::CStr; -use std::mem::MaybeUninit; +use std::mem::{self, MaybeUninit}; use std::ptr; use std::slice; use std::str; @@ -769,12 +769,8 @@ impl<'a> CryptoBufferBuilder<'a> { // Make sure all bytes in buffer initialized as required by Boring SSL. return Err(ErrorStack::internal_error_str("invalid len")); } - unsafe { - let mut result = ptr::null_mut(); - ptr::swap(&mut self.buffer, &mut result); - std::mem::forget(self); - Ok(result) - } + // Drop is no-op if the buffer is null + Ok(mem::replace(&mut self.buffer, ptr::null_mut())) } }