From 53c84a4ba5d986388bfdc759886cb4e6cbf70339 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 28 Jan 2025 17:20:12 +0000 Subject: [PATCH 1/5] fix(bindings): avoid connection free during unwind --- bindings/rust/extended/s2n-tls/src/callbacks.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index 5d871c34844..d689dddbed6 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -51,14 +51,15 @@ where F: FnOnce(&mut Connection, &mut Context) -> T, { let raw = NonNull::new(conn_ptr).expect("connection should not be null"); - let mut conn = Connection::from_raw(raw); + // Since this is a callback, it receives a pointer to the connection + // but doesn't own that connection or control its lifecycle. + // Do not drop / free the connection. + // We must make the connection `ManuallyDrop` before `action`, otherwise a panic + // in `action` would cause the unwind mechanism to drop the connection. + let mut conn = ManuallyDrop::new(Connection::from_raw(raw)); let mut config = conn.config().expect("config should not be null"); let context = config.context_mut(); 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. - // Do not drop / free the connection. - let _ = ManuallyDrop::new(conn); r } @@ -100,3 +101,9 @@ pub(crate) unsafe fn verify_host( Err(_) => 0, // If the host name can't be parsed, fail closed. } } + +#[cfg(test)] +mod tests { + // if a customer + fn panic_is_graceful() +} \ No newline at end of file From 908ec5a6f7f2fe148e9b2f98ef8d7c9e5f7eb40a Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 28 Jan 2025 18:56:00 +0000 Subject: [PATCH 2/5] add testcase --- .../rust/extended/s2n-tls/src/callbacks.rs | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index d689dddbed6..8065dcd64dd 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -53,7 +53,7 @@ where let raw = NonNull::new(conn_ptr).expect("connection should not be null"); // Since this is a callback, it receives a pointer to the connection // but doesn't own that connection or control its lifecycle. - // Do not drop / free the connection. + // Do not drop / free the connection. // We must make the connection `ManuallyDrop` before `action`, otherwise a panic // in `action` would cause the unwind mechanism to drop the connection. let mut conn = ManuallyDrop::new(Connection::from_raw(raw)); @@ -104,6 +104,42 @@ pub(crate) unsafe fn verify_host( #[cfg(test)] mod tests { - // if a customer - fn panic_is_graceful() -} \ No newline at end of file + use crate::{ + callbacks::with_context, + config::{Config, Context}, + connection::{Builder, Connection}, + enums::Mode, + }; + + // The temporary connection created in `with_context` should never be freed, + // even if customer code panics. + #[test] + fn panic_does_not_free_connection() -> Result<(), crate::error::Error> { + fn immediate_panic(_conn: &mut Connection, _context: &mut Context) { + panic!("a panic in customer code"); + } + + let config = Config::new(); + assert_eq!(config.test_get_refcount().unwrap(), 1); + let mut connection = config.build_connection(Mode::Server)?; + + // 1 connection + 1 self reference + assert_eq!(config.test_get_refcount()?, 2); + + let conn_ptr = connection.as_ptr(); + let unwind_result = std::panic::catch_unwind(|| { + unsafe { with_context(conn_ptr, immediate_panic) }; + }); + + // a panic happened + assert!(unwind_result.is_err()); + + // the connection hasn't been freed yet + assert_eq!(config.test_get_refcount()?, 2); + + drop(connection); + assert_eq!(config.test_get_refcount()?, 1); + + Ok(()) + } +} From 94a66684782f0b2521cea4cba3c68488a704bf47 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 28 Jan 2025 19:01:31 +0000 Subject: [PATCH 3/5] use context instead of context_mut --- bindings/rust/extended/s2n-tls/src/callbacks.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index 8065dcd64dd..821d79937ee 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -48,7 +48,7 @@ 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"); // Since this is a callback, it receives a pointer to the connection @@ -57,8 +57,8 @@ where // We must make the connection `ManuallyDrop` before `action`, otherwise a panic // in `action` would cause the unwind mechanism to drop the connection. let mut conn = ManuallyDrop::new(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); r } @@ -115,12 +115,7 @@ mod tests { // even if customer code panics. #[test] fn panic_does_not_free_connection() -> Result<(), crate::error::Error> { - fn immediate_panic(_conn: &mut Connection, _context: &mut Context) { - panic!("a panic in customer code"); - } - let config = Config::new(); - assert_eq!(config.test_get_refcount().unwrap(), 1); let mut connection = config.build_connection(Mode::Server)?; // 1 connection + 1 self reference @@ -128,7 +123,11 @@ mod tests { let conn_ptr = connection.as_ptr(); let unwind_result = std::panic::catch_unwind(|| { - unsafe { with_context(conn_ptr, immediate_panic) }; + unsafe { + with_context(conn_ptr, |_conn, _context| { + panic!("force unwind"); + }) + }; }); // a panic happened From 7bbcbc8ae35f5c1d18a1e1286bfef845dfa0e28f Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 28 Jan 2025 19:23:46 +0000 Subject: [PATCH 4/5] clippy & fmt --- bindings/rust/extended/s2n-tls/src/callbacks.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index 821d79937ee..1a710673604 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -59,8 +59,7 @@ where let mut conn = ManuallyDrop::new(Connection::from_raw(raw)); let config = conn.config().expect("config should not be null"); let context = config.context(); - let r = action(&mut conn, context); - r + action(&mut conn, context) } /// A trait for the callback used to verify host name(s) during X509 @@ -104,12 +103,7 @@ pub(crate) unsafe fn verify_host( #[cfg(test)] mod tests { - use crate::{ - callbacks::with_context, - config::{Config, Context}, - connection::{Builder, Connection}, - enums::Mode, - }; + use crate::{callbacks::with_context, config::Config, connection::Builder, enums::Mode}; // The temporary connection created in `with_context` should never be freed, // even if customer code panics. From 8b5d1afc0fda23c8d78c364e3a4d4838172c3298 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 30 Jan 2025 00:24:10 +0000 Subject: [PATCH 5/5] Revert "use context instead of context_mut" This reverts commit 94a66684782f0b2521cea4cba3c68488a704bf47. --- bindings/rust/extended/s2n-tls/src/callbacks.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bindings/rust/extended/s2n-tls/src/callbacks.rs b/bindings/rust/extended/s2n-tls/src/callbacks.rs index 1a710673604..c1d476ec1b8 100644 --- a/bindings/rust/extended/s2n-tls/src/callbacks.rs +++ b/bindings/rust/extended/s2n-tls/src/callbacks.rs @@ -48,7 +48,7 @@ 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, &Context) -> T, + F: FnOnce(&mut Connection, &mut Context) -> T, { let raw = NonNull::new(conn_ptr).expect("connection should not be null"); // Since this is a callback, it receives a pointer to the connection @@ -57,8 +57,8 @@ where // We must make the connection `ManuallyDrop` before `action`, otherwise a panic // in `action` would cause the unwind mechanism to drop the connection. let mut conn = ManuallyDrop::new(Connection::from_raw(raw)); - let config = conn.config().expect("config should not be null"); - let context = config.context(); + let mut config = conn.config().expect("config should not be null"); + let context = config.context_mut(); action(&mut conn, context) }