From c3035961eb5eddad0c861dd978592851eb731b42 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 29 Jan 2025 06:46:56 +0000 Subject: [PATCH 1/5] fix(bindings): make Context borrow immutable --- bindings/rust/extended/s2n-tls/src/callbacks.rs | 6 +++--- bindings/rust/extended/s2n-tls/src/renegotiate.rs | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index 5d871c34844..4701977a68b 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -48,12 +48,12 @@ pub use pkey::*; /// callbacks were configured through the Rust bindings. pub(crate) unsafe fn with_context(conn_ptr: *mut s2n_connection, action: F) -> T where - F: FnOnce(&mut Connection, &mut Context) -> T, + F: FnOnce(&mut Connection, &Context) -> T, { let raw = NonNull::new(conn_ptr).expect("connection should not be null"); let mut conn = Connection::from_raw(raw); - let mut config = conn.config().expect("config should not be null"); - let context = config.context_mut(); + let config = conn.config().expect("config should not be null"); + let context = config.context(); let r = action(&mut conn, context); // Since this is a callback, it receives a pointer to the connection // but doesn't own that connection or control its lifecycle. diff --git a/bindings/rust/extended/s2n-tls/src/renegotiate.rs b/bindings/rust/extended/s2n-tls/src/renegotiate.rs index 215ca630c42..e1388b1603e 100644 --- a/bindings/rust/extended/s2n-tls/src/renegotiate.rs +++ b/bindings/rust/extended/s2n-tls/src/renegotiate.rs @@ -41,7 +41,7 @@ //! //! impl RenegotiateCallback for Callback { //! fn on_renegotiate_request( -//! &mut self, +//! &self, //! conn: &mut Connection, //! ) -> Option { //! let response = match conn.server_name() { @@ -51,7 +51,7 @@ //! Some(response) //! } //! -//! fn on_renegotiate_wipe(&mut self, conn: &mut Connection) -> Result<(), Error> { +//! fn on_renegotiate_wipe(&self, conn: &mut Connection) -> Result<(), Error> { //! conn.set_application_protocol_preference(Some("http"))?; //! Ok(()) //! } @@ -156,7 +156,7 @@ pub trait RenegotiateCallback: 'static + Send + Sync { // This method returns Option instead of Result because the callback has no mechanism // for surfacing errors to the application, so Result would be somewhat deceptive. fn on_renegotiate_request( - &mut self, + &self, connection: &mut Connection, ) -> Option; @@ -165,13 +165,13 @@ pub trait RenegotiateCallback: 'static + Send + Sync { /// Because renegotiation requires wiping the connection, connection-level /// configuration will need to be set again via this callback. /// See [`Connection::wipe_for_renegotiate()`] for more information. - fn on_renegotiate_wipe(&mut self, _connection: &mut Connection) -> Result<(), Error> { + fn on_renegotiate_wipe(&self, _connection: &mut Connection) -> Result<(), Error> { Ok(()) } } impl RenegotiateCallback for RenegotiateResponse { - fn on_renegotiate_request(&mut self, _conn: &mut Connection) -> Option { + fn on_renegotiate_request(&self, _conn: &mut Connection) -> Option { Some(*self) } } @@ -390,7 +390,7 @@ impl config::Builder { response: *mut s2n_renegotiate_response::Type, ) -> libc::c_int { with_context(conn_ptr, |conn, context| { - let callback = context.renegotiate.as_mut(); + let callback = context.renegotiate.as_ref(); if let Some(callback) = callback { if let Some(result) = callback.on_renegotiate_request(conn) { // If the callback indicates renegotiation, schedule it. @@ -718,7 +718,7 @@ mod tests { struct ErrorRenegotiateCallback {} impl RenegotiateCallback for ErrorRenegotiateCallback { fn on_renegotiate_request( - &mut self, + &self, _: &mut Connection, ) -> Option { None From 33347b0c2af19ccb56f4e799574d506e2fa82300 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 30 Jan 2025 00:39:46 +0000 Subject: [PATCH 2/5] rustfmt --- bindings/rust/extended/s2n-tls/src/renegotiate.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/renegotiate.rs b/bindings/rust/extended/s2n-tls/src/renegotiate.rs index e1388b1603e..4f8dfc809e6 100644 --- a/bindings/rust/extended/s2n-tls/src/renegotiate.rs +++ b/bindings/rust/extended/s2n-tls/src/renegotiate.rs @@ -155,10 +155,7 @@ pub trait RenegotiateCallback: 'static + Send + Sync { // // This method returns Option instead of Result because the callback has no mechanism // for surfacing errors to the application, so Result would be somewhat deceptive. - fn on_renegotiate_request( - &self, - connection: &mut Connection, - ) -> Option; + fn on_renegotiate_request(&self, connection: &mut Connection) -> Option; /// A callback that triggers after the connection is wiped for renegotiation. /// @@ -717,10 +714,7 @@ mod tests { fn error_callback() -> Result<(), Box> { struct ErrorRenegotiateCallback {} impl RenegotiateCallback for ErrorRenegotiateCallback { - fn on_renegotiate_request( - &self, - _: &mut Connection, - ) -> Option { + fn on_renegotiate_request(&self, _: &mut Connection) -> Option { None } } From 6a0d13e4e4cf087066ff099016196fb54befb37c Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Sat, 1 Feb 2025 04:03:24 +0000 Subject: [PATCH 3/5] address pr feedback * make context_mut unsafe --- bindings/rust/extended/s2n-tls/src/config.rs | 32 +++++++++++--------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/config.rs b/bindings/rust/extended/s2n-tls/src/config.rs index c289b1bf0a8..be3b2e6531f 100644 --- a/bindings/rust/extended/s2n-tls/src/config.rs +++ b/bindings/rust/extended/s2n-tls/src/config.rs @@ -93,14 +93,16 @@ impl Config { /// Retrieve a mutable reference to the [`Context`] stored on the config. /// /// Corresponds to [s2n_config_get_ctx]. - pub(crate) fn context_mut(&mut self) -> &mut Context { + /// + /// SAFETY: There must only ever by mutable reference to `Context` alive at + /// any time. Configs can be shared across threads, so this method is + /// almost certainly not correct for your usecase. + unsafe fn context_mut(&mut self) -> &mut Context { let mut ctx = core::ptr::null_mut(); - unsafe { - s2n_config_get_ctx(self.as_mut_ptr(), &mut ctx) - .into_result() - .unwrap(); - &mut *(ctx as *mut Context) - } + s2n_config_get_ctx(self.as_mut_ptr(), &mut ctx) + .into_result() + .unwrap(); + &mut *(ctx as *mut Context) } #[cfg(test)] @@ -135,7 +137,7 @@ impl Clone for Config { impl Drop for Config { /// Corresponds to [s2n_config_free]. fn drop(&mut self) { - let context = self.context_mut(); + let context = self.context(); let count = context.refcount.fetch_sub(1, Ordering::Release); debug_assert!(count > 0, "refcount should not drop below 1 instance"); @@ -158,13 +160,15 @@ impl Drop for Config { // https://github.com/rust-lang/rust/blob/e012a191d768adeda1ee36a99ef8b92d51920154/library/alloc/src/sync.rs#L1637 std::sync::atomic::fence(Ordering::Acquire); - unsafe { - // This is the last instance so free the context. - let context = Box::from_raw(context); - drop(context); + // This is the last instance so free the context. + let context = unsafe { + // SAFETY: The reference count is verified to be 1, so this is the + // last instance of the config, and the only reference to the context. + Box::from_raw(self.context_mut()) + }; + drop(context); - let _ = s2n_config_free(self.0.as_ptr()).into_result(); - } + let _ = unsafe { s2n_config_free(self.0.as_ptr()).into_result() }; } } From b4012418f028a81e73907ba6b5d40f3f6e77918c Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 5 Feb 2025 18:49:50 +0000 Subject: [PATCH 4/5] remove context_mut() getter on builder --- bindings/rust/extended/s2n-tls/src/config.rs | 80 ++++++++++++------- .../rust/extended/s2n-tls/src/renegotiate.rs | 6 +- 2 files changed, 58 insertions(+), 28 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/config.rs b/bindings/rust/extended/s2n-tls/src/config.rs index be3b2e6531f..8ac63cf6b54 100644 --- a/bindings/rust/extended/s2n-tls/src/config.rs +++ b/bindings/rust/extended/s2n-tls/src/config.rs @@ -96,7 +96,7 @@ impl Config { /// /// SAFETY: There must only ever by mutable reference to `Context` alive at /// any time. Configs can be shared across threads, so this method is - /// almost certainly not correct for your usecase. + /// likely not correct for your usecase. unsafe fn context_mut(&mut self) -> &mut Context { let mut ctx = core::ptr::null_mut(); s2n_config_get_ctx(self.as_mut_ptr(), &mut ctx) @@ -338,7 +338,12 @@ impl Builder { ) .into_result() }; - self.context_mut().application_owned_certs.push(chain); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because the + // Builder owns the only reference to the config. + self.config.context_mut() + }; + context.application_owned_certs.push(chain); result?; Ok(self) @@ -377,9 +382,12 @@ impl Builder { let collected_chains = chain_arrays.into_iter().take(cert_chain_count).flatten(); - self.context_mut() - .application_owned_certs - .extend(collected_chains); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because the + // Builder owns the only reference to the config. + self.config.context_mut() + }; + context.application_owned_certs.extend(collected_chains); unsafe { s2n_config_set_cert_chain_and_key_defaults( @@ -551,12 +559,17 @@ impl Builder { verify_host(host_name, host_name_len, handler) } - self.context_mut().verify_host_callback = Some(Box::new(handler)); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because the + // Builder owns the only reference to the config. + self.config.context_mut() + }; + context.verify_host_callback = Some(Box::new(handler)); unsafe { s2n_config_set_verify_host_callback( self.as_mut_ptr(), Some(verify_host_cb_fn), - self.context_mut() as *mut Context as *mut c_void, + self.config.context() as *const Context as *mut c_void, ) .into_result()?; } @@ -611,7 +624,11 @@ impl Builder { } let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.client_hello_callback = Some(handler); unsafe { @@ -632,7 +649,11 @@ impl Builder { ) -> Result<&mut Self, Error> { // Store callback in config context let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.connection_initializer = Some(handler); Ok(self) } @@ -663,14 +684,18 @@ impl Builder { // Store callback in context let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.session_ticket_callback = Some(handler); unsafe { s2n_config_set_session_ticket_cb( self.as_mut_ptr(), Some(session_ticket_cb), - self.context_mut() as *mut Context as *mut c_void, + self.config.context() as *const Context as *mut c_void, ) .into_result() }?; @@ -702,7 +727,11 @@ impl Builder { } let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.private_key_callback = Some(handler); unsafe { @@ -738,13 +767,17 @@ impl Builder { } let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.wall_clock = Some(handler); unsafe { s2n_config_set_wall_clock( self.as_mut_ptr(), Some(clock_cb), - self.context_mut() as *mut _ as *mut c_void, + self.config.context() as *const _ as *mut c_void, ) .into_result()?; } @@ -777,13 +810,17 @@ impl Builder { } let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.monotonic_clock = Some(handler); unsafe { s2n_config_set_monotonic_clock( self.as_mut_ptr(), Some(clock_cb), - self.context_mut() as *mut _ as *mut c_void, + self.config.context() as *const _ as *mut c_void, ) .into_result()?; } @@ -917,17 +954,6 @@ impl Builder { pub(crate) fn as_mut_ptr(&mut self) -> *mut s2n_config { self.config.as_mut_ptr() } - - /// Retrieve a mutable reference to the [`Context`] stored on the config. - pub(crate) fn context_mut(&mut self) -> &mut Context { - let mut ctx = core::ptr::null_mut(); - unsafe { - s2n_config_get_ctx(self.as_mut_ptr(), &mut ctx) - .into_result() - .unwrap(); - &mut *(ctx as *mut Context) - } - } } #[cfg(feature = "quic")] diff --git a/bindings/rust/extended/s2n-tls/src/renegotiate.rs b/bindings/rust/extended/s2n-tls/src/renegotiate.rs index 4f8dfc809e6..06b770b68fb 100644 --- a/bindings/rust/extended/s2n-tls/src/renegotiate.rs +++ b/bindings/rust/extended/s2n-tls/src/renegotiate.rs @@ -405,7 +405,11 @@ impl config::Builder { } let handler = Box::new(handler); - let context = self.context_mut(); + let context = unsafe { + // SAFETY: usage of context_mut is safe in the builder, because while + // it is being built, the Builder is the only reference to the config. + self.config.context_mut() + }; context.renegotiate = Some(handler); unsafe { s2n_config_set_renegotiate_request_cb( From 8949fae52a4d0c784414c7ee5c312d2619e3b79c Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 18 Feb 2025 21:42:44 +0000 Subject: [PATCH 5/5] mark items as pub(crate) to allow for separate renegotiate module --- bindings/rust/extended/s2n-tls/src/config.rs | 4 ++-- bindings/rust/extended/s2n-tls/src/renegotiate.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/config.rs b/bindings/rust/extended/s2n-tls/src/config.rs index 8ac63cf6b54..75d63bbab47 100644 --- a/bindings/rust/extended/s2n-tls/src/config.rs +++ b/bindings/rust/extended/s2n-tls/src/config.rs @@ -97,7 +97,7 @@ impl Config { /// SAFETY: There must only ever by mutable reference to `Context` alive at /// any time. Configs can be shared across threads, so this method is /// likely not correct for your usecase. - unsafe fn context_mut(&mut self) -> &mut Context { + pub(crate) unsafe fn context_mut(&mut self) -> &mut Context { let mut ctx = core::ptr::null_mut(); s2n_config_get_ctx(self.as_mut_ptr(), &mut ctx) .into_result() @@ -173,7 +173,7 @@ impl Drop for Config { } pub struct Builder { - config: Config, + pub(crate) config: Config, load_system_certs: bool, enable_ocsp: bool, } diff --git a/bindings/rust/extended/s2n-tls/src/renegotiate.rs b/bindings/rust/extended/s2n-tls/src/renegotiate.rs index 06b770b68fb..08f5dc8fe2a 100644 --- a/bindings/rust/extended/s2n-tls/src/renegotiate.rs +++ b/bindings/rust/extended/s2n-tls/src/renegotiate.rs @@ -245,8 +245,8 @@ impl Connection { // We trigger the callback last so that the application can modify any // preserved configuration (like the server name or waker) if necessary. - if let Some(mut config) = self.config() { - if let Some(callback) = config.context_mut().renegotiate.as_mut() { + if let Some(config) = self.config() { + if let Some(callback) = config.context().renegotiate.as_ref() { callback.on_renegotiate_wipe(self)?; } }